Streamerbot / client

Streamer.bot WebSocket client
https://streamerbot.github.io/client/
MIT License
23 stars 4 forks source link

Potential memory leak #7

Open meza opened 2 days ago

meza commented 2 days ago

Hey,

Newer node versions have started notifying about suboptimal handling of event emitters. This is Node 20.5.0

image

Instead of blindly increasing the limit, I've dug into the actual problem it's notifying about and it comes back to the request method the lib uses under the hood.

image

Basically every time a request is made, a new eventListener to the message is assigned without ever cleaning them up. Normally with websockets, you only ever attach a single eventListener to the existing socket, and that listener does all the processing and dispatching of the work.

The current implementation feels like it's trying to mimic a traditional HTTP request roundtrip. You could try removing the attached listener but that might actually be more work and error prone than refactoring it to behave more like a websocket implementation

Whipstickgostop commented 2 days ago

Request-response is intended here and is an often used valid pattern in WebSockets.

These should, however, be cleaning themselves up and it was an oversight if they are not 👍

Whipstickgostop commented 2 days ago

abort() is being called after any pass or fail of the request which should then remove the listener.

Do you have any additional evidence of listeners building up over time?

11 listeners is not a lot and the new Node warning is mostly useful in instances like this to be sure nothing is wrong and then increase the limit within the library itself.