child_process: null channel handle on close#3041
Closed
indutny wants to merge 2 commits intonodejs:masterfrom
Closed
child_process: null channel handle on close#3041indutny wants to merge 2 commits intonodejs:masterfrom
null channel handle on close#3041indutny wants to merge 2 commits intonodejs:masterfrom
Conversation
`HandleWrap::OnClose` destroys the underlying C++ object and null's the internal field pointer to it. Therefore there should be no references to the wrapping JavaScript object. `null` the process' `_channel` field right after closing it, to ensure no crashes will happen. Fix: nodejs#2847
Member
Author
|
cc @nodejs/collaborators |
indutny
added
the
child_process
Contributor
There was a problem hiding this comment.
common should be loaded first. It has some global pollution detection logic.
Member
Author
|
@thefourtheye fixed, thanks! |
Contributor
|
LGTM |
Contributor
There was a problem hiding this comment.
Looking at the surrounding code, without this loop also the assertion in close event will be satisfied, right?
Member
Author
There was a problem hiding this comment.
The worker won't exit without the loop. It expect the message to come. Sending multiple items is crucial to make this test fail on the unpatched node.
Member
Author
|
@thefourtheye may I ask you to finish the review ;) |
Contributor
|
LGTM |
Member
Author
|
Thanks! Going to run CI and land it. |
Member
Author
Member
Author
|
Landed in 36b969f, thank you! |
indutny
added a commit
that referenced
this pull request
Sep 25, 2015
`HandleWrap::OnClose` destroys the underlying C++ object and null's the internal field pointer to it. Therefore there should be no references to the wrapping JavaScript object. `null` the process' `_channel` field right after closing it, to ensure no crashes will happen. Fix: #2847 PR-URL: #3041 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
indutny
added a commit
that referenced
this pull request
Sep 25, 2015
`HandleWrap::OnClose` destroys the underlying C++ object and null's the internal field pointer to it. Therefore there should be no references to the wrapping JavaScript object. `null` the process' `_channel` field right after closing it, to ensure no crashes will happen. Fix: #2847 PR-URL: #3041 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This was referenced Sep 30, 2015
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
HandleWrap::OnClosedestroys the underlying C++ object and null's theinternal field pointer to it. Therefore there should be no references to
the wrapping JavaScript object.
nullthe process'_channelfield right after closing it, to ensureno crashes will happen.
Fix: #2847