client tracking bug in handleUpgrade about websocket.io

[rauchg]

That's awful.

[rauchg]

Although client tracking usage is discouraged, I forgot to add it to the README

[rauchg]

It actually looks like it's potentially a race condition instead.

[rauchg]

@nicokaiser
I cleaned up some of your clientTracking work here: https://github.com/LearnBoost/websocket.io/compare/fix-tracking
Is there a chance for a race condition here? Can a client reserve a i and then by the time open fires the i is a different client?

[railnerd]

I had assumed the use of "i" was a typo, not a reference to the other "i" in the module.

[railnerd]

actually, the other issue is that you end up with a clients array full of nulls. Couldn't you track clients using an associative array instead?

if (self.options.clientTracking) {
  self.clients[client] = client;
  self.clientsCount++;
  client.on('close', function () {
    delete self.clients[client];
    self.clientsCount--;
  });
}

[railnerd]

(and yes I agree that fix was god awful) ;-)

[rauchg]

No I meant an awful bug haha!! Thanks for reporting this

[railnerd]

BTW: I still think there is a leak where the clients array never shrinks. I'm a fairly recent student of Javascript so apologies if I get any of this wrong, I'm an OCD C programmer by day— I started playing with node & socket.io on a hobby project and have been smitten— haven't had this much fun programming since 5th grade (and trust me, that was a LONG time ago).

[rauchg]

Yes. We should delete instead of setting to null

[rauchg]

And clients should be a {}

[ruxkor]

While inspecting the same blocks because of issue 42, I think that using an object is actually not the best solution for it:

While the access to single server.clients would be efficient, slicing, etc. would be much more cumbersome. The other point is that even with the object usage, i would steadily increase.

In commit 68f8258f97 in my fork I addressed this issue by

• saving the closed connections (i.e. the ones that will be set to null) in an array, and
• reusing these indices when a new connection is created

this way, the maximum array size is bound to the maximum number of concurrent connections, which seems to be the optimal tradeoff for my uses.

[rauchg]

I like that approach, would you mind submitting a pull request? Thanks!

[ruxkor]

here it is :) the changes are only about the array filling, without your fix-tracking branch (i.e. it differs from the commit I linked to in the last comment).