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

test: fix race in parallel/test-vm-debug-context #1294

Merged
merged 1 commit into from
Mar 29, 2015

Conversation

bnoordhuis
Copy link
Member

Fix a race condition in parallel/test-vm-debug-context where the 'exit'
event for the child process is emitted before the first and only 'data'
event for the child process's stderr stream.

I considered deferring the 'exit' event in lib/child_process.js until
all stdio streams have been closed but I realized that's not going to
work when the child process spins off grandchildren that keep the stdio
file descriptors alive.

Fixes: #1291

R=@brendanashworth

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/402/

@mscdex mscdex added the test Issues and PRs related to the tests. label Mar 29, 2015
@brendanashworth
Copy link
Contributor

Awesome! I can't get it to fail anymore on my laptop, but the CI looks like it died. LGTM.

Fix a race condition in parallel/test-vm-debug-context where the 'exit'
event for the child process is emitted before the first and only 'data'
event for the child process's stderr stream.

I considered deferring the 'exit' event in lib/child_process.js until
all stdio streams have been closed but I realized that's not going to
work when the child process spins off grandchildren that keep the stdio
file descriptors alive.

Fixes: nodejs#1291
PR-URL: nodejs#1294
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@bnoordhuis bnoordhuis closed this Mar 29, 2015
@bnoordhuis bnoordhuis deleted the fix-issue-1291 branch March 29, 2015 22:30
@bnoordhuis bnoordhuis merged commit 8d1c87e into nodejs:v1.x Mar 29, 2015
@bnoordhuis
Copy link
Member Author

Thanks. I ran the CI a second time just in case and Jenkins is happy this time around: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/403/

@rvagg rvagg mentioned this pull request Mar 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants