Skip to content
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

"drain" event listener leak when using res.once("drain"); can't use res.removeListener("drain") #152

Closed
zbjornson opened this issue Mar 7, 2019 · 3 comments · May be fixed by #153
Closed

Comments

@zbjornson
Copy link
Contributor

This module intercepts res.on and remaps it to stream.on. Using res.once("drain", listener) multiple times creates a listener leak because this is essentially what happens:

res.once("drain", listener);
// expands to this (see https://github.com/nodejs/node/blob/aec34473a7eddae32e6c359715ce521446066210/lib/events.js#L282-L303)
res.on("drain", () => {
  res.removeListener("drain", listener);
  listener();
});
// but compression intercepts `on`, so you wind up with:
stream.on("drain", () => {
  res.removeListener("drain", listener); // <-- wrong target
  listener();
})

For that same reason, it looks like a listener bound with res.on("drain", listener) can never actually be removed with res.removeListener.

(Also mentioned in #135 (comment).)

@dougwilson
Copy link
Contributor

duplicate of #135

please correct me if this is not a duplicate ticket

@zbjornson
Copy link
Contributor Author

I guess it's the same underlying cause, but two new manifestations.

@dougwilson
Copy link
Contributor

Gotcha. I assume that the Node.js .unpipe logic is just calling removeListener under the hood, in which case the fix being to fixup removeListener would fix these cases, I would guess. It seems this issue is mentioned in #135 which would help drive whoever picks up the bug to fix to get it covered in a generic way. You could add your above snippet to that issue as well, if you think that would be useful to someone who picks up #135

zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 7, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 8, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 8, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 8, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 12, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 12, 2019
zbjornson added a commit to zbjornson/compression that referenced this issue Mar 12, 2019
dougwilson pushed a commit to zbjornson/compression that referenced this issue Jul 16, 2020
dougwilson pushed a commit to zbjornson/compression that referenced this issue Jul 16, 2020
dougwilson pushed a commit to zbjornson/compression that referenced this issue Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants