On Close bind not removed from UDP port when using relay

[pabjr]

First of all, nice piece of work.

The issue that I am highlighting is encountered when using the UDP to WebSocket example. In the example the relay is created after the UDP port is brought up and the on the WebSocket connections event:

    let socketPort = new osc.WebSocketPort({
        socket: socket,
        metadata: true
    });

    let relay = new osc.Relay(udp, socketPort, {
        raw: true
    });

This creation is done every time a new socket is added by a browser. Therefore it is possible to have multiple relays for the same UDP port. For the most part this works well and supports the multiple connections over multiple browser refreshes.

The problem I found was if the number of connections or web page refreshes exceeds 11 Node will warn of a memory leak.

(node:6957) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [EventEmitter]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit
(Use node --trace-warnings ... to show where the warning was created)

When I traced this behavior I found that the problem lie in the relay code. The code was binding the relay "close" event to the port "close" event to make sure that the relay would be released when a port closed. This works for Websockets that are temporary but it causes a build up of "close" events on the UDP port.

All that said, I think I have found a work around but am not sure if it is the most elegant or the best way to prevent the build up or where I should suggest its inclusion.

What I did was removed the "close" listener from the port during the execution of the "stopRelaying" function:

osc.stopRelaying = function (relay, from, relaySpec){ 
      from.removeListener(relaySpec.eventName,relaySpec.listener);
      from.removeListener("close", relay.closure);
};  

This required me to add a property to the relay object

this.closure = this.close.bind(this);
this.port1.on("close", this.closure);
this.port2.on("close", this.closure);

and pass the relay object to the stopRelaying function

osc.stopRelaying(this,this.port1, this.port1Spec);
osc.stopRelaying(this,this.port2, this.port2Spec);

So, I would like to know if this is a good way to handle this issue and where is the best place in the code to keep the change on my system. Right now I have modified the osc-transport.js file.

Also on a different topic, could you point me to an example on how to trap malformed OSC errors to prevent and exception fault.

Best Regards,

[colinbdclark]

Hi @pabjr, I'm glad to hear you're liking osc.js. Honestly, osc.relay was never designed for anything other than non-trivial use, and I probably shouldn't have created examples that use it. It's undocumented and listed in the source as an "unsupported, non-API function" for this reason. My apologies, I know that the examples are a bit misleading.

It does seem, though, that there is is a bug in osc.relay, you're right. Wouldn't the simplest implementation, though, involve removing the relay's close listeners from both ports in the close() method? So I think you're right that we need to hold on to a reference to the bound close function when we create it:

p.listen = function () {
   ...

    this.closeListener = this.close.bind(this);
    this.port1.on("close", this.closeListener);
    this.port2.on("close", this.closeListener);
};

And then in close() you should be able to remove the listener:

p.close = function () {
    osc.stopRelaying(this.port1, this.port1Spec);
    osc.stopRelaying(this.port2, this.port2Spec);
    this.emit("close", this.port1, this.port2);
    this.port1.removeListener("close", this.closeListener);
    this.port2.removeListener("close", this.closeListener);
};

This is just typed out directly and untested, but since you've got an application that already exhibits this issue, could you give it a shot and see if something like this addresses the issue, and I can make a fix and release?

As for malformed OSC messages, are you finding that listening for the error event isn 't working for you?

I hope this helps, and thanks so much for pointing this issue out!

[pabjr]

@colinbdclark Thanks for the rewrite. I can confirm it does work and cleans things up a bit.

Your note also raises another question. When you said that ocs.relay was not mean for anything other than non-trivial use, what did you mean? Should I be approaching the conversion of OCS UDP message to a web app differently?

Regarding the error event example, I had seen it but wasn't sure if it was what I needed. In the relay example I placed it right after the upd.on ready code and it seems to work. Thanks again for the quick response.

I sure I will be back as I work through my implementation. ;)

[colinbdclark]

Glad to hear confirmation that it works. I'll roll this fix into an upcoming release.

As far as osc.relay, I think it's fine to use, just keep in mind that you might find more bugs or unexpected cases than in the rest of the API. I know others use it quite extensively (as have I) and it was ok.

Good luck with your implementation, and please feel free to share anything you've created with the library!