child_process: null channel handle on close
#3041
Closed
Conversation
This file contains 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
`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
|
cc @nodejs/collaborators |
indutny
added
the
child_process
| 'use strict'; | ||
|
|
||
| const assert = require('assert'); | ||
| const common = require('../common'); |
There was a problem hiding this comment.
common should be loaded first. It has some global pollution detection logic.
|
@thefourtheye fixed, thanks! |
|
LGTM |
| server.close(); | ||
| })); | ||
|
|
||
| // Queue up several handles, to make `process.disconnect()` wait |
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?
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.
|
@thefourtheye may I ask you to finish the review ;) |
|
LGTM |
|
Thanks! Going to run CI and land it. |
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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