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

http.globalAgent.maxSockets is not always respected #4050

Closed
cyrus-and opened this issue Nov 27, 2015 · 3 comments
Closed

http.globalAgent.maxSockets is not always respected #4050

cyrus-and opened this issue Nov 27, 2015 · 3 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@cyrus-and
Copy link

Aborting a pending request (whose response is not read yet) causes http.globalAgent.maxSockets to not being respected for future requests.

Specifically:

  • the response must contain some data;
  • there must be a 'response' listener registered for the requests.

I can reproduce the issue with the following:

client.js:

var http = require('http');

http.globalAgent.maxSockets = 3;

for (var i = 0; i < 100; i++) {
    var request = http.get('http://127.0.0.1:8080', function () {
        // just register a listener...
    });
    request.setTimeout(1000, function () {
        console.log('T');
        this.abort();
    });
}

server.js:

require('http').createServer(function (reqest, response) {
    console.log('R');
    response.end('hello');
}).listen(8080);

Run server.js then client.js; the output of the former is:

R
R
R
# 1s timeout here...
R
R
R
R
R
R
# 1s timeout here...
R
R
R
R
R
R
R
R
R
R
R
R
# 1s timeout here...
[...]
# up to 100

I was expecting groups of three requests each, instead of: 100 = 3 + 6 + 12 + 24 + 48 + 7.
The output of client.js reflects the one of server.js.

This happen at least with v4.1.1 and v5.1.0.


I apologize if the above is the intended behavior and I'm just misunderstanding request.abort() and agent.maxSockets.

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Nov 27, 2015
@claudiorodriguez
Copy link
Contributor

It doesn't look like request.abort() frees the sockets in the agent. I tried keepAlive + maxFreeSockets but that didn't work either (all requests were sent at once).

Here's how I was able to get your example to work:

var http = require('http');

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

var reqConfig = {
    host: 'localhost',
    port: 8080,
    agent: agent
};

var i, cntRes = 0;
for (i = 0; i < 100; i++) {
    var request = http.get(reqConfig, function () {
        console.log(cntRes++);
        // just register a listener...
    });

    request.on("socket", function (socket) {
        socket.setTimeout(1000, function() {
           socket.end();
        });
    });
}

EDIT: socket.destroy() also works. I'm looking at request.abort's code and in theory it should call the same method (calls this.socket.destroy). So no idea why this is happening.

davidvgalbraith pushed a commit to davidvgalbraith/node that referenced this issue Dec 21, 2015
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in nodejs#4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.
indutny pushed a commit that referenced this issue Dec 21, 2015
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in #4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: #4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@mscdex
Copy link
Contributor

mscdex commented Dec 21, 2015

This should be fixed by 6e11e22. Can you verify @cyrus-and?

@cyrus-and
Copy link
Author

@mscdex everything is working as expected now, thanks.

Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Dec 22, 2015
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in nodejs#4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: nodejs#4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Jan 6, 2016
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in nodejs#4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: nodejs#4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 13, 2016
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in #4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: #4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 19, 2016
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in #4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: #4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in nodejs#4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: nodejs#4172
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants