support res.removeListener('drain'), res.once('drain')

[zbjornson]

Fixes #152
Fixes #135

This issue turned out not to be a duplicate of #135; unpiping still causes a leak. With latest changes, it is fixed

[dougwilson]

Awesome, thank you! I'm just using this as a reminder for one of us (I can help when I have some time a bit later) make the tests actually not pass when the issue is there, as otherwise the CI state is not useful.

[zbjornson]

Thanks for the fast turn-around. Fixed the issue you pointed out, and made it so tests actually fail. (Proxying listenerCount() would be nice, but seems sorta hard and wouldn't directly test the underlying issue. e.g. would you count buffered listeners?)

[dougwilson]

Hi @zbjornson sorry; I didn't mean to make a new review, I was just still in the middle of reviewing and since the code changed underneath, github discarded all my in progress comments and i had to start a new review. i still need to retype a few of my comments, but need to leave the office right now, but will be back on later to finish retyping them 👍

[dougwilson]

p.s. (on mobile to write this) i just wanted to thank you for helping put this together. will get it out asap when it lands, as it's an important fix that i feel bad i never got around to fixing myself. pr is very appreciated

[zbjornson]

Sorry for the force-push while you were reviewing!

Meanwhile, I just pushed fixes for the comments so far.

prependListener(), prependOnceListener(), removeAllListeners() I think still have the potential to leak. Not sure how far to go with this PR. I'm not sure it's even safe to use removeAllListeners() in this scenario.

rawListeners(), listeners(), listenerCount(), eventNames() are also sorta broken, but at least won't leak (and are fairly rare APIs).

Thanks! :)

[dougwilson]

Yea, i think it would be nice to fix everything, but doesn't need to be in this pr if not desired. Was mostly just looking for (a) fix existing leak and (b) not introduce new leaks. If there are other leaks still, for instance, and they are not fixed, it's not a new one :)

For example if removealllisteners is broken currently, this pr doesnt change that, so can be a separate fix, if desired. But the addlistener was just that it would have worked no leak and this would make it leak, which is why i brought it up, if that makes sense in my thought process

[zbjornson]

@dougwilson anything else you need from me on this PR?

[paularmstrong]

@dougwilson Sorry to bump this, but it looks like it's been forgotten about for a year now. We've been seeing memory leaks related to this in our production environment and would really love to see this merged in. Anything we can do to help get it in?

[bambooCZ]

We're experiencing this too. Reproducible like this

const express = require('express');
const compression = require('compression');

const app = express();
const port = 8080;

// comment-out following line to make the problem disappear
app.use(compression());

// Some data to serve
const data = `long data long data long data long data\n`; 

app.get('/', async (req, res) => {
    res.set('content-type', 'text/plain; charset=utf-8');

    for (let i = 0; i < 10000; i++) {
        if (res.write(data)) continue;
        await new Promise((resolve) => res.once('drain', resolve));
    }

    res.end();
});

app.listen(port, () => console.log(`Example app listening at http://localhost:${port}`));

[zbjornson]

@dougwilson I think this PR is still ready to land. Despite the label, it has test coverage.

[dougwilson]

Apologies for that (sitting and not updating the label). I will take a look tonight and ideally get it landed tonight for everyone. I also want to see it land :) but so many things go on sometimes loose track.

[dougwilson]

I'm working on a change for this, as it seems to have the potential to break some other things that may be around, so just wanted to be careful around this. The idea for the original .on was to act AOP-style, adding an around modifier. This PR attempts to extend that on modified to the addListener method, but the issue with the current impl before I change it is that it assumes that having res.addListener suddenly call res.on will no create issues. I don't think it will, but it certainly has the protentional to introduce weird issues with other using AOP, maybe to debug. For instance, a hypothetical app that replaces res.addListener to perhaps log out when it calls will never see it happen when this module is in use, as any res.addListener override will be erased.

I'm working to fix that and will be pushing up a separate commit to the PR to address this, so be on the look out for it and check my work :)