benchmark: terminate child process on Windows#12658
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
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. Refs: nodejs#12560
Trott added benchmark 
nodejs-github-bot added benchmark 
MemberAuthor
Trott commented Apr 26, 2017
CI stress test on master branch showing failures: https://ci.nodejs.org/job/node-stress-single-test/nodes=win2016-1p/1171/console CI stress test on this branch showing success: https://ci.nodejs.org/job/node-stress-single-test/1174/nodes=win2016-1p/console |
MemberAuthor
Trott commented Apr 26, 2017
/cc @bzoz (who proposed the fix in #12560 (comment)) |
MemberAuthor
Trott commented Apr 26, 2017
refack reviewed Apr 26, 2017
| bench.end(bytes); | ||
| if(process.platform==='win32'){ | ||
| // Sometimes there's a yes.exe process left hanging around on Windows... | ||
| child_process.execSync(`taskkill /f /t /pid ${child.pid}`); |
Contributor
There was a problem hiding this comment.
Is this the best way to do it, it seems so cruel 😭
Now seriously, child.kill should work, aren't we masking a real problem?
Contributor
There was a problem hiding this comment.
NM, yes.exe is a ported unix util, it's probably crazy
Contributor
refack commented Apr 26, 2017
how about using |
refack approved these changes Apr 26, 2017
| bench.end(bytes); | ||
| if(process.platform==='win32'){ | ||
| // Sometimes there's a yes.exe process left hanging around on Windows... | ||
| child_process.execSync(`taskkill /f /t /pid ${child.pid}`); |
Contributor
There was a problem hiding this comment.
NM, yes.exe is a ported unix util, it's probably crazy
Contributor
refack commented Apr 26, 2017
Best solution would be to use something less crazy then |
bzoz approved these changes Apr 26, 2017
jasnell approved these changes Apr 26, 2017
Trott added a commit to Trott/io.js that referenced this pull request Apr 28, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: nodejs#12658 Ref: nodejs#12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
MemberAuthor
Trott commented Apr 28, 2017
Landed in a1a54ca. |
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 3, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
refack added a commit to refack/node that referenced this pull request May 4, 2017
2 tasks
refack added a commit to refack/node that referenced this pull request May 6, 2017
PR-URL: nodejs#12821Fixes: nodejs#12817 Refs: nodejs#12658 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
Member
Should this be backported to v6.x? |
MemberAuthor
Trott commented May 16, 2017
Yes assuming the relevant benchmark exists there etc. |
gibfahn pushed a commit that referenced this pull request May 16, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: #12658 Ref: #12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
PR-URL: nodejs#12821Fixes: nodejs#12817 Refs: nodejs#12658 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #12821Fixes: #12817 Refs: #12658 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #12821Fixes: #12817 Refs: #12658 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
test-benchmark-child-process failures reveal that child-process-exec-stdout benchmark sometimes leaves around a stray yes.exe process. Add code to terminate the process. PR-URL: nodejs/node#12658 Ref: nodejs/node#12560 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: James M Snell <[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.
test-benchmark-child-process failures reveal that
child-process-exec-stdout benchmark sometimes leaves around a stray
yes.exe process. Add code to terminate the process.
Refs: #12560
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test benchmark child_process