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.

Sign up for GitHub

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

Jump to bottom

HTTP agent: remove excess calls to removeSocket #4172

Closed
wants to merge 1 commit into from
Closed

HTTP agent: remove excess calls to removeSocket #4172

wants to merge 1 commit into from

Conversation

davidvgalbraith
Copy link

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.

@r-52 r-52 added the http Issues or PRs related to the http subsystem. label Dec 6, 2015
@mscdex
Copy link
Contributor

mscdex commented Dec 6, 2015

CI was all green except unrelated, flaky tests on Windows and pi1-raspbian-wheezy had this test failure:

not ok 46 test-http-agent-maxsockets-regress-4050.js
# events.js:143
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: socket hang up
#     at createHangUpError (_http_client.js:210:15)
#     at Socket.socketCloseListener (_http_client.js:242:23)
#     at emitOne (events.js:84:20)
#     at Socket.emit (events.js:171:7)
#     at TCP._onclose (net.js:470:12)

I'm not sure if it's a flaky test or what?

@davidvgalbraith
Copy link
Author

Found it -- calling abort() on a request before it has triggered the response event gives you a socket hangup. Replacing that flimsy timeout with a response event handler ought to make it bulletproof.

@davidvgalbraith
Copy link
Author

Hey! Been a few days, anything else I can do to give this one a kick?

@mscdex
Copy link
Contributor

mscdex commented Dec 10, 2015

CI again: https://ci.nodejs.org/job/node-test-pull-request/974/

@mscdex
Copy link
Contributor

mscdex commented Dec 10, 2015

LGTM, the couple of tests that did fail are known to be flaky.

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

@nodejs/http

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

LGTM but would like additional review from @nodejs/http

@indutny
Copy link
Member

indutny commented Dec 11, 2015

I see one possible problem here. close event is emitted on a next libuv tick. But considering that the .sockets is used just for bookkeeping, it should be safe.

LGTM

@dougwilson
Copy link
Member

LGTM

@davidvgalbraith
Copy link
Author

Universal acclaim! Let's get this in?

@davidvgalbraith
Copy link
Author

Bump! (hope it's not too pestilential to bump)

@indutny
Copy link
Member

indutny commented Dec 21, 2015

@davidvgalbraith I would like to land this patch, but may I ask you to review this contributing guidelines first: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit, and make sure that the commit log matches them?

Namely there are at least two issues with the messages right now:

Cheers!

HTTP: remove excess calls to removeSocket
8bfe887 
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.
@davidvgalbraith
Copy link
Author

Ok, I changed the subsystem in the title to HTTP and made the body lines ~72 characters each.

@indutny
Copy link
Member

indutny commented Dec 21, 2015

Landed in 6e11e22, thank you!

@indutny indutny closed this Dec 21, 2015
indutny pushed a commit that referenced this pull request Dec 21, 2015
@indutny
http: remove excess calls to removeSocket
6e11e22 
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]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
@Fishrock123
http: remove excess calls to removeSocket
8168669 
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 pull request Jan 6, 2016
@Fishrock123
http: remove excess calls to removeSocket
8f7af9a 
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 pull request Jan 13, 2016
http: remove excess calls to removeSocket
44bb763 
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 pull request Jan 19, 2016
http: remove excess calls to removeSocket
39dc054 
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 MylesBorins mentioned this pull request Jan 19, 2016
Merged
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
http: remove excess calls to removeSocket
d7bf666 
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]>
@leonardoadame leonardoadame mentioned this pull request May 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@davidvgalbraith @mscdex @jasnell @indutny @dougwilson @MylesBorins @r-52