MatrixAI / js-quic

QUIC Networking for TypeScript & JavaScript
https://matrixai.github.io/js-quic/
Apache License 2.0
13 stars 1 forks source link

fix `MaxListenersExceededWarning` event handler 'leak' #80

Closed tegefaulkes closed 7 months ago

tegefaulkes commented 7 months ago

Specification

When a lot of connections are made we get the following warning

(node:371624) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 EventQUICSocketStopped listeners added to EventTarget. Use events.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)

This is not strictly a problem, it's a dumb warning if we register too many handlers to a single event.

I did some digging in the code and found the cause to be us registering a once handler to EventQUICSocketStopped in createQUICCLient. Normally this is not a problem unless you're using a shared socket and making many connections. Which is our normal usage in Polykey.

This is not really a bug in our usage, It should be fine to register multiple events to a single handler like this. But we need to remove the warning and I'm not sure what the best ways to go about it are. So I'm creating this issue to discuss it.

The options are

  1. Remove the handler. Simplest but the client wouldn't protectively handle the socket stopping anymore.
  2. Replace the event handling with a callback the QUICSocket calls. Same difference but the socket will need to track the clients to make calls to them. Far from ideal.
  3. Remove the handler and have something, say the NodeConnectionManager handle the event and make calls to the QUICClients. Then it's up to whatever manages the clients to handle the event. Meaning we depend on external usage to handle this.

Any other ideas?

I've made some changes and testing in the feature-EventTarget-leak branch

Additional context

Tasks

  1. Determine the best fix
  2. Implement the fix.
tegefaulkes commented 7 months ago

We had a quick discussion about it. This is the one case were it's fine to have many handlers on the one event. Since we have many clients to one socket. So the solution here is to just up the limit before the warning is shown. Keep in mind that this will hide future problems. Also, I'm pretty sure using events.setMaxListeners() will apply globally to anything that imports js-quic as well.

tegefaulkes commented 7 months ago

This was addressed by commit d3a1d937f43d24cb065510b2abb78c9e0374630c

tegefaulkes commented 7 months ago

On reflection The solution is too global. I'm going to look into this a bit more.

tegefaulkes commented 7 months ago

Ok, so there is a way to set it specifically for an instance of EventTarget using the following.

    const {
      setMaxListeners,
    } = require('events');
    console.log(sharedSocket[_eventTarget]);
    setMaxListeners(100000, sharedSocket[_eventTarget]);

I had to do a little digging to find that function.

I'll be applying it to the QUICSocket EventTarget.

tegefaulkes commented 7 months ago

OK, the warning limit should only be increased for the QUICSocket EventTarget.

Marking this as closed as per commit d5abdb2afdffbac6ea64fe7ab80d7fbe5110a7d6

CMCDragonkai commented 7 months ago

I'd argue you should put that in a utility function somewhere and expose it. But also consider conditionally running that while in nodejs platform. Not sure since we would want to use in non-node platforms.