Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

ClientRequest setTimeout EventEmitter Leak #3068

Closed
pselden opened this Issue · 3 comments

4 participants

@pselden

Running the following code yields the trace warning:

(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace:
at Socket. (events.js:139:15)
at Socket.once (events.js:160:8)
at Socket.setTimeout (net.js:143:12)
at ClientRequest. (http.js:1287:29)
at ClientRequest.g (events.js:156:14)
at ClientRequest.emit (events.js:88:20)
at Array.0 (http.js:1273:9)
at EventEmitter._tickCallback (node.js:192:40)

    var http = require('http');

    var port = 12345;
    var server = http.createServer(function(req, res) {
      setTimeout(function(){
          res.writeHead(200, {'Content-Type': 'text/plain'});
          res.end('OK');
      }, 10);
    });

    var agent = new http.Agent({maxSockets: 1});

    server.listen(port, function() {
      for(var i = 0; i < 11; ++i){
        createRequest().end();
      }

      function createRequest(){
        var req = http.request({port: port, path: '/', agent: agent});

        req.setTimeout(1000, function(){});
        return req;
      }
    });

The timeout event is being listened to on the underlying socket (via once), but when timeout is not fired, it is obviously not being cleared. The user must clear it manually via req.socket.removeListener('timeout), but this exposes underlying implementation details.

I looked at the code to see if I could patch it myself, but it wasn't clear to me when I'd need to clear it (on response, on error, on end???). At the very least we could add req.clearTimeout(fn) so the underlying implementation details don't have to be leaked to clear the timeout.

@ssuda ssuda referenced this issue from a commit in ssuda/node
ssuda Fixing ClientRequest setTimeout EventEmitter Leak
This will fix #3068
08f6a15
@isaacs
Owner

Adding a clearTimeout method is an ugly API that makes it even more confusing with the global setTimeout/clearTimeout functions, when really, it's a completely different thing.

The issue here is that we ought to remove the timeout listeners when it's clear that the timeout event will never happen, and not add multiple listeners when it's clear that the connection has already not timed out, since it's a shared connection.

@pselden

Agreed re: clearTimeout being sub-optimal... it's just less sub-optimal than the way I'm able to clearing the timeout via leaking into the socket implementation. I couldn't see one centralized place in the code where it says "i'm done with this request" (either via error, or actually completing), and adding it multiple would turn it into spaghetti.

@bts bts referenced this issue from a commit in tms-engineering/node
ssuda Fixing ClientRequest setTimeout EventEmitter Leak
This will fix #3068
f2c112d
@hitsthings

This issue seems to have resurfaced. I just updated from 0.6.15 to 0.8.9 and this started happening for me.

My current workaround is using the global setTimeout/clearTimeout rather than req.setTimeout. I'm actually using https though, so the stack is slightly different:

(node) warning: possible EventEmitter memory leak detected. 11 listeners added.
Use emitter.setMaxListeners() to increase limit.
Trace
at Socket.EventEmitter.addListener (events.js:168:15)
at Socket.EventEmitter.once (events.js:189:8)
at Socket.setTimeout (net.js:174:12)
at CleartextStream.CryptoStream.setTimeout (tls.js:269:32)
at ClientRequest.setTimeout (http.js:1557:17)
at ClientRequest.setTimeout (http.js:1569:10)
at ClientRequest.g (events.js:185:14)
at ClientRequest.EventEmitter.emit (events.js:115:20)
at ClientRequest.onSocket (http.js:1514:9)
at process.startup.processNextTick.process._tickCallback (node.js:244:9)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.