expressjs / compression

Node.js compression middleware
MIT License
2.77k stars 241 forks source link

"drain" event listener leak when unpiping to response #135

Open zbjornson opened 6 years ago

zbjornson commented 6 years ago

This is sort of a weird scenario, but this test case fails:

  it('should clean up event listeners when res is unpiped to', function (done) {
    var listenerCount
    var server = createServer({ threshold: 0 }, function (req, res) {
      var times = 0
      var int = setInterval(function () {
        var rs = fs.createReadStream('does not exist')
        rs.on('error', function (e) {
          listenerCount = res.listenerCount('drain')
          rs.unpipe(res)
        })
        rs.pipe(res)
        if (times++ > 12) {
          clearInterval(int)
          res.end('hello, world')
        }
      })
    })

    request(server)
      .get('/')
      .set('Accept-Encoding', 'gzip')
      .expect(function () {
        assert.ok(listenerCount < 2)
      })
      .expect(200, done)
  })

I hit this in some code that retries creating a read stream until the source exists. We clean up from our side: rs.on("error", e => { rs.unpipe("res"); }). Seems like compression needs to be cleaning up its listeners when "unpipe" happens.

matthiasg commented 6 years ago

there is also a leak caused by mapping ServerResponse.once to ServerResponse.on so there is no easy way to wait for 'drain' events to happen by just using the response.once('drain', ...)

jesseskinner commented 2 years ago

If anyone came here looking for a workaround for the ability to call res.once('drain', ...) without have node warn about MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 drain listeners added to [Gzip]. Use emitter.setMaxListeners() to increase limit..

This is a bit of a mess, but you can do something like this:

let onDrain;

// add a single drain listener early on
res.on('drain', () => {
    if (onDrain) {
        // call the onDrain callback at most once
        onDrain();
        onDrain = undefined;
    }
});

// later, when you would normally call res.once('drain', ...)
onDrain = () => {
    // resume your input stream or whatever
};

I hope that helps someone feeling stuck on this old issue.