WebSocket object leakage in WebSocketServer.clients when using WebSocket.onclose = function(..){..}

[jacobbogers]

Hi,
I discovered a leakage in WebSocket.clients when using callbacks like so for WebSocket

 ws.onclose = function close(evt) {
           ..
}

In the code I discovered that on connection on the server side a callback is registered to remove the peer from the WebSocket.clients Set

Unfortunately this hidden callback is removed when using ws.onclose = function style.

Since this is a cleanup operation, (after closure of socket) I changed the 'hidden' emit event name to "cleanup".

this will prevent the hidden callback removal when using ws.onclose == function style callback
(Look at how onclose property is defined, in removes all event handlers , so also any hidden "close" event handlers)

[jacobbogers]

excuse me it seems to fail checks on node 4 but node 6 and 7 are ok, please advise
locally on my computer it passes all checks

[lpinca]

@jacobbogers good catch, don't worry about the failure on node 4, it is not related to this change.
I wonder if we can fix the issue by changing how browser API emulation work. The problem is that ws.onclose() = () => {} calls ws.removeAllListeners('close') so maybe it's better to fix that.

[jacobbogers]

yes I know the property removes all listeners on "onclose" set property,
that is a good thing, and is compliant with web frontend behavior (like onload , onerror, etc)

but hidden "close" to cleanup is just piggyback

better to make separate event for cleanup also it intent is made clear,
alternative you can not work with events but callback functions WebSocket ====> (optionally calls cleanup on callback on)===> WebSocketServer but it is essentially the same thing

[jacobbogers]

maybe call the event "remove-from-queue"?

[lpinca]

@jacobbogers I was thinking about something like this:

diff --git a/lib/WebSocket.js b/lib/WebSocket.js
index 4baf5a8..08a1f5d 100644
--- a/lib/WebSocket.js
+++ b/lib/WebSocket.js
@@ -409,8 +409,11 @@ WebSocket.CLOSED = 3;
      * @public
      */
     get () {
-      const listener = this.listeners(method)[0];
-      return listener ? listener._listener ? listener._listener : listener : undefined;
+      const listeners = this.listeners(method);
+
+      for (var i = 0; i < listeners.length; i++) {
+        if (listeners[i]._listener) return listeners[i]._listener;
+      }
     },
     /**
      * Add a listener for the event.
@@ -419,7 +422,12 @@ WebSocket.CLOSED = 3;
      * @public
      */
     set (listener) {
-      this.removeAllListeners(method);
+      const listeners = this.listeners(method);
+
+      for (var i = 0; i < listeners.length; i++) {
+        if (listeners[i]._listener) this.removeListener(method, listeners[i]);
+      }
+
       this.addEventListener(method, listener);
     }
   });

[jacobbogers]

ok, so you tag a listener with _listener attribute
I see it in your return this.listeners(method).find((listener) => listener._listener);

and when you "set" you loop over all existing listener and remove the one with _listener attribute

So the hidden 'close' would NOT have the _listener attribute tag

I understand the solution, you "tag" all "user defined" listener with _listener property.

I now see distinction between addEventListener/addListener and removeEventListener/removeListener

Ok, yes , if you choose this route then good to document why you make exception in
if (listener._listener) statement

My style is separate message stream for separate cleanup functionality

[lpinca]

Yes, it's about reusing what we already have as listeners added via on<event> already have a _listener property.

That said, your approach is definitely easier to understand.

[lpinca]

The only reason why I'm a bit hesitant about using a specific event is that this event should ideally be emitted only when clientTracking is enabled while close is already emitted in all cases.

[jacobbogers]

Is good too leverage what is already there, I suggest add 2 lines of documentation one for the "close" and one for the if statement. Since we talk about "clients" collection, I see you change it too set, my code uses "Array.length" because it is array now, so will soon need to change it to "Set.size" )))
Can you abstract what kind of collection you use ?

[jacobbogers]

Shall i make your suggested changed? then i get some kuddos for finding the bug and first fix attempt)))

[lpinca]

@jacobbogers sure! Go for it.

[lpinca]

Since we talk about "clients" collection, I see you change it too set, my code uses "Array.length" because it is array now, so will soon need to change it to "Set.size" )))
Can you abstract what kind of collection you use ?

The next version will be a major (2.0.0). Breaking changes will be documented.

[lpinca]

@jacobbogers ping.

[lpinca]

This needs a regression test but I'll add it in a bit.
Thanks!