Search code examples
javascriptnode.jstcpmodule

JS: Maximum call stack size exceeded on disconnect despite function being empty

I am kinda new to js and experimenting with the net module.

I have a simple server running, that distributes packages to all known clients but as soon as one client disconnects the server crashes on the 'end' event despite this function only including a log command:

var clients = [];

const server = net.createServer((c) => {

  // 'connection' listener
  console.log("client connected")
  clients.push(c);

  c.on('end', () => {
    console.log('client disconnected');
    //dc(c)
  });

  c.on('error', () => {
    c.write("400");
  })

  c.on('data', (data) => {

    console.log(data.toString())
    writeAll(c, data);

  });

});

Write all is a function below that distributes the incoming package to all known clients, excluding c:

function writeAll(exclude, buffer) {

  clients.forEach((client) => {
    if(client != exclude) {
      client.write(buffer);
    }
  });

}

dc() is a function that is supposed to delete the client from the array list I defined earlier but is commented out so it should not matter. Everything works fine, until one client disconnects, which is, when I get the following error message:

internal/errors.js:207
function getMessage(key, args) {
                   ^

RangeError: Maximum call stack size exceeded
    at getMessage (internal/errors.js:207:20)
    at new NodeError (internal/errors.js:153:13)
    at doWrite (_stream_writable.js:411:19)
    at writeOrBuffer (_stream_writable.js:399:5)
    at Socket.Writable.write (_stream_writable.js:299:11)
    at Socket.c.on (C:\Users\...\server.js:17:7)
    at Socket.emit (events.js:197:13)
    at errorOrDestroy (internal/streams/destroy.js:98:12)
    at onwriteError (_stream_writable.js:430:5)
    at onwrite (_stream_writable.js:461:5)

The referenced line 17 is the on - error event you can see above. I already tried extending the max stack length to 2000 but it still does not work. The weirdest thing is, the part throwing the error message is copy-pasted from another code I know works for a fact.

I would really appreciate if one of you pros out there would take this on since I am completely lost.

Edit: The full file can be found here: https://ghostbin.com/paste/knm3f

Edit 2: I reinstalled the net module and now the error changed, leaving me even more confused:

internal/errors.js:222
  const expectedLength = (msg.match(/%[dfijoOs]/g) || []).length;
                              ^
RangeError: Maximum call stack size exceeded
    at String.match (<anonymous>)
    at getMessage (internal/errors.js:222:31)
    at new NodeError (internal/errors.js:153:13)
    at doWrite (_stream_writable.js:411:19)
    at writeOrBuffer (_stream_writable.js:399:5)
    at Socket.Writable.write (_stream_writable.js:299:11)
    at Socket.c.on (C:\Users\...\server.js:17:7)
    at Socket.emit (events.js:197:13)
    at errorOrDestroy (internal/streams/destroy.js:98:12)
    at onwriteError (_stream_writable.js:430:5)

Edit 3: The code that definitely did work has been tested and for some reason runs into the same problem even on the machine it was written on, as well as my laptop. I'm getting the feeling this is not on me somehow.

Edit 4: SOOOOOOOOOOO... It turns out Microsoft has once again put it's fist really far up my rear end. It works 100 fine on a Linux distro. I would put my bets on a Windows firewall change or some other update I did not agree to. Thank you Microsoft. You are forcing me to migrate to Linux.

Edit 5: The error on disconnect was only thrown on windows, but the stackoverflow was caused by the error event writing to the client, which itseld threw an error, since the client was not connected anymore.

Solution

  • There are two bugs in your code and they're definitely not too obvious.

    Whenever you write to a client, you do so with this loop in writeAll():

    clients.forEach((client) => {
  if(client != exclude) {
    client.write(buffer);
  }
});

    Here we see that you do client.write(...). That command may detect that this specific client closed its connection. That means an emit('error') is going to happen and your corresponding callback will get called.

    In that callback, you are calling dc(c) which is expected to remove the client from the array:

    clients.splice(index, 1)

    In other words, you are modifying the array named clients while you are doing a clients.forEach(). This is a big no no! I actually try to forget about that forEach() function altogether. It has created so many problems on my end! Some people say forEach() is also slower than for() or while() loops. But that problem may have been improved.

    What you want to do, at least, is use a for() loop:

    for(let i = 0; i < clients.length; ++i)
{
   ...
}

    Notice that I do not first save the clients.length in a variable since it could change over time!

    The problem with that approach is that whenever a splice() occurs, you're going to miss some clients.

    The next best thing to do is to use a loop going backward. For this a while() is well adapted:

    let i = clients.length
while(i > 0)
{
    --i
    ...
}

    This loop will work better as long as: (1) only one items gets splice()d out, (2) no new client gets added to the list.

    The final, strong solution, the one I would propose is to copy the array first:

    const client_list = clients.slice()
client_list.forEach((client) => { ... })    // if you really want to use the forEach()?!

    The copy is not going to change: it is local and const.

    But why would the stack get messed up from that? Probably because as a result of the error you do a write() to the client... Okay, this is not related to the forEach() but this code loops:

    c.on('error', () => {
    c.write("400")     // are you missing a "\n", btw?
});

    As you can see, the c.write() is the culprit of the error being generated, you're going to loop forever in this one on() callback... You need to check whether c is valid or catch errors:

    c.on('error', () => {
    try
    {
        c.write("400")     // are you missing a "\n", btw?
    }
    catch(err)
    {
        // maybe log the error?
        console.log(err)
    }
});

    That being said, I know from experience that after I removed all forEach() calls from my code and made copies of arrays (objects) that I can't be sure whether they would not get modified under my feet, my code worked much better.