2

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.

PixelRayn
  • 392
  • 2
  • 13
  • 1
    The exact point that a Stack Overflow happens at is usually random. What's the full stack trace? There must be more. You have infinite recursion somewhere. – Carcigenicate Feb 14 '19 at 16:20
  • Are you saying that it fails when `dc(c)` is *not* commented out? If so, what exactly does `dc()` look like? – Pointy Feb 14 '19 at 16:21
  • That is the full stack trace and dc(c) is commented out, so it should not matter. – PixelRayn Feb 14 '19 at 16:24
  • Probably the issue is that you're doing something in an event handler (like the `.on()` callback) that triggers the same event, resulting in infinite recursion. – Pointy Feb 14 '19 at 16:24
  • @PixelRayn There must be more. The browser must be truncating the trace. See if you can get the full printout. – Carcigenicate Feb 14 '19 at 16:26
  • 1
    Not using a browser. this is a node application. @Carcigenicate – PixelRayn Feb 14 '19 at 16:28
  • @Carcigenicate, by default the last 10 calls are shown, this has been like that for a while... – Alexis Wilke Feb 14 '19 at 18:54
  • Often the best way to diagnose difficult recursion problems is to get a Stack trace. Unless you can obtain more detail, you'll be left trying to figure it out manually. See if you can find a setting to change how much detail it prints. And like I mentioned, the exact function that it fails it is usually meaningless. The exact function that it fails at will depend on how full the stack is before the recursion starts, among other factors. That's the same error/problem; the circumstances are just slightly different causing it to fail at a different call. – Carcigenicate Feb 14 '19 at 19:02
  • I don't use Node, but I'm quite familiar with recursion. @Alexis You should mention how this can be diagnosed if you're familiar with Node. – Carcigenicate Feb 14 '19 at 19:08
  • @Carcigenicate It always fails on the same function. When I exit. This is an error in System. Might have something to do with the environment, since the old code stopped working from one day to the other, without any changes being made to it. Imma start this up on a Linux live distro. – PixelRayn Feb 14 '19 at 20:18
  • @Carcigenicate, yes. A full stack trace could be useful. I'm not sure whether it would be in this case. It's not rare, also, that the stack lies in various cases, especially on stack & memory related errors (memory errors should be rare in Node.js though.) As for how long you can make it, there is some info about that here: https://stackoverflow.com/questions/7697038/more-than-10-lines-in-a-node-js-stack-error – Alexis Wilke Feb 14 '19 at 23:25

1 Answers1

1

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.

Alexis Wilke
  • 19,179
  • 10
  • 84
  • 156