This repository has been archived by the owner. It is now read-only.

ClientRequest setTimeout EventEmitter Leak #3068

Closed
pselden opened this Issue Apr 6, 2012 · 3 comments

Comments

Projects
None yet
4 participants
@pselden

pselden commented Apr 6, 2012

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 pushed a commit to ssuda/node that referenced this issue Apr 8, 2012

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs Apr 10, 2012

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.

isaacs commented Apr 10, 2012

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

This comment has been minimized.

Show comment Hide comment
@pselden

pselden Apr 11, 2012

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.

pselden commented Apr 11, 2012

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 added a commit to tms-engineering/node that referenced this issue May 11, 2012

@hitsthings

This comment has been minimized.

Show comment Hide comment
@hitsthings

hitsthings Sep 24, 2012

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)

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 subscribe to this conversation on GitHub. Already have an account? Sign in.