-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Prevent unsubscribe from leaking listeners (#1295) #1474
fix: Prevent unsubscribe from leaking listeners (#1295) #1474
Conversation
Since 8eeeec1 it seems listeners have been leaking on keep-alive sockets. Apparently abort() has been deprecated since v17, using close() the onSocketClose listener is properly removed, and by creating a new onData function I'm able to remove the 'data' listener too.
ping @LinusU to take a look |
Removing of request.on('abort', () => {
socket.removeListener('close', onSocketClose);
}); is OK, It was added in 51861e9 on Aug 12, 2021 by @tekwiz Upd: Line 101 in 004b3ac
But anyway it worth to use Also maybe it makes sense to use |
The other change is also the correct one. Removing the The current code works obviously not-properly in case when the same socket is used for multiple requests. It will add additional listeners on each request. The current implementation only suited for non keep-alive Agent — one request, one socket. |
BTW, the other way to prevent the existent of multiple listeners on the same /** @type {WeakSet<net.Socket>} */
const socketsWithOnDataListener = new WeakSet();
// ...
if (!socketsWithOnDataListener.has(socket)) {
socket.prependListener("data", onData);
} Did not test, but should work. Upd: |
🎉 This PR is included in version 3.2.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Could this fix be backported to the 2.x line? Would a PR doing that be accepted/considered? Context: #1295 (comment) user reported this in 2.6.0, and openai/openai-node#349 also reported this with (Further context, I help maintain the EDIT: per this comment from Jimmy which says that bugfixes will be accepted to the 2.x branch, I'll see if we can put up a PR for that soon. |
Since 8eeeec1 it seems listeners have been leaking on keep-alive sockets.
Apparently abort() has been deprecated since v17, using close() the onSocketClose listener is properly removed, and by creating a new onData function I'm able to remove the 'data' listener too.