Use a set instead of an array to track clients

[lpinca]

I think it makes sense to use a Set instead of an Array to track clients.
The main advantage is that Set.prototype.delete() complexity is sublinear per spec.

[JoshuaWise]

I agree that this should be the case. However, not before the next major version.

In fact, ideally, we make a subclass of Set, where the non-readonly methods are stubbed.

[lpinca]

There are already quite a few breaking changes in master.

On 21 Sep 2016, at 16:56, Joshua Wise [email protected] wrote:

I agree that this should be the case. However, not before the next major version.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[3rd-Eden]

Was about to point out that now that we're using class and other ES6 features in master we might as well add this.

[JoshuaWise]

Okay, then my next concern it with my comment above:
Ideally, we make a subclass of Set, where the non-readonly methods are stubbed.

[lpinca]

Why is a subclass needed?

[JoshuaWise]

I suppose a subclass specifically isn't needed. But the non-readonly methods should be stubbed nonetheless. Otherwise unsuspecting users (people who think the returned Set is a safe copy) won't be at risk of silent memory leaks

[lpinca]

@JoshuaWise I don't get it, mind to write a code example of the issue?

[JoshuaWise]

wss.on('connection', function (client) {
    wss.clients.delete(client); // I shouldn't be allowed to do this.
});

[JoshuaWise]

It's very common for people to expect "safe copies" of arrays or sets. Meaning, they expect that the object that is returned will be a copy of the internally-kept original one, which allows them to modify it or do whatever they want with it. However, in our case, this is not the case. The clients object is not a safe copy, so we should explicitly prevent people from modifying the object.

[lpinca]

I don't see it that way. I think all is needed is documentation. If the end users want to mess with the set they should be free to do so as long as it is a public API. It is already like this with the array.

[JoshuaWise]

When would the user ever want to actually mess with it?

I know that's how it is with the array—I see it as an oversight that should be fixed

[lpinca]

I can't think of a use case atm but I also can't see why it should be forbidden.
If you know that a set is returned and you know that it is not a copy it's your responsibility to not mess with it.

[3rd-Eden]

Once it's rebased id say it's clear for merging.

[3rd-Eden]

As for the deletion, people can already do that in the current way it's setup as well. Just clients.length = 0 and everything is broken. I'd say we have some trust in our users and they know what they are doing when they enable client tracking as it's disabled by default.

[3rd-Eden]

Why not check if this.clients is set instead of checking for the options?

[lpinca]

I don't even remember these changes 😄. I'll take a look!

[3rd-Eden]

The only change i'd make here is maybe move the try/catch to the inside of the loop so if one client fails to terminate we still try to kill the rest of them.