http: remove reference to onParserExecute#4773
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed.
Contributor
mscdex commented Jan 20, 2016
/cc @indutny |
Contributor
mscdex commented Jan 20, 2016
Member
bnoordhuis commented Jan 20, 2016
LGTM. For mechanical sympathy it might be good to also null the property in the freelist factory function on line 231 in |
Member
indutny commented Jan 20, 2016
LGTM + what @bnoordhuis said |
Contributor
mscdex commented Jan 20, 2016
LGTM |
Member
bnoordhuis commented Jan 20, 2016
zz85 commented Jan 21, 2016
@Nibbler999 thanks for finding this fix! As mentioned in #4779, this seems to be of help tested against MacOS NodeJS. While looking through this patch, along other comments I found lines 182 to be somewhat interesting too :) // TODO: All parser data should be attached to a// single object, so that it can be easily cleaned// up by doing `parser.data ={}`, which should// be done in FreeList.free. `parsers.free(parser)`// should be all that is needed. |
ChALkeR added the memory bnoordhuis pushed a commit that referenced this pull request Jan 21, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: #4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>Member
bnoordhuis commented Jan 21, 2016
Landed in 5f6dfab, thanks. Tagging for v5.x and LTS. |
rvagg pushed a commit that referenced this pull request Jan 25, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: #4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>
Contributor
MylesBorins commented Feb 17, 2016
this must land with #4337 |
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: #4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: #4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: #4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Parsers hold a reference to the socket associated with the request through onParserExecute. This must be removed when the parser is freed so that the socket can be garbage collected when destroyed. Regression introduced in commit 59b91f1 ("http_parser: consume StreamBase instance"). PR-URL: nodejs#4773 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parsers hold a reference to the socket associated with the request
through onParserExecute. This must be removed when the parser is
freed so that the socket can be garbage collected when destroyed.
Not removing this causes the last 1000 sockets used for inbound http requests to go uncollected after being destroyed.
Code was introduced in 59b91f1.