◐ Shell
reader mode source ↗
Skip to content

benchmark: terminate child process on Windows#12658

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:flaky-benchmark-fix
Closed

benchmark: terminate child process on Windows#12658
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:flaky-benchmark-fix

Conversation

@Trott

@Trott Trott commented Apr 26, 2017

Copy link
Copy Markdown
Member

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test benchmark child_process

@Trott
benchmark: terminate child process on Windows …
b42cd1c 
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 Trott added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Apr 26, 2017
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. labels Apr 26, 2017
@Trott

Trott commented Apr 26, 2017

Copy link
Copy Markdown
Member Author

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

@Trott

Trott commented Apr 26, 2017

Copy link
Copy Markdown
Member Author

/cc @bzoz (who proposed the fix in #12560 (comment))

@Trott

Trott commented Apr 26, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/7682/

@refack

refack commented Apr 26, 2017

Copy link
Copy Markdown
Contributor

how about using child_process.execFile(file[, args][, options][, callback]) in line 25 instead of exec
I know it changes the benchmark, but isn't it a better surrogate?

@refack

refack commented Apr 26, 2017

Copy link
Copy Markdown
Contributor

Best solution would be to use something less crazy then yes

Trott added a commit to Trott/io.js that referenced this pull request Apr 28, 2017
@Trott
benchmark: terminate child process on Windows …
a1a54ca 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

Trott commented Apr 28, 2017

Copy link
Copy Markdown
Member Author

Landed in a1a54ca.

@Trott Trott closed this Apr 28, 2017
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@Trott @evanlucas
benchmark: terminate child process on Windows …
dbe9882 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@Trott @evanlucas
benchmark: terminate child process on Windows …
780fa39 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@Trott @evanlucas
benchmark: terminate child process on Windows …
0ae986b 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 3, 2017
@Trott @evanlucas
benchmark: terminate child process on Windows …
224fd3a 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack added a commit to refack/node that referenced this pull request May 4, 2017
@refack
test: fix flakyness with yes.exe …
7e8f7c7 
Fixes: nodejs#12817
Ref: nodejs#12658
@refack refack mentioned this pull request May 4, 2017
Closed
2 tasks
refack added a commit to refack/node that referenced this pull request May 6, 2017
@refack
test: fix flakyness with yes.exe …
68c933c 
PR-URL: nodejs#12821
Fixes: nodejs#12817
Refs: nodejs#12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn

gibfahn commented May 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Should this be backported to v6.x?

@Trott

Trott commented May 16, 2017

Copy link
Copy Markdown
Member Author

Should this be backported to v6.x?

Yes assuming the relevant benchmark exists there etc.

gibfahn pushed a commit that referenced this pull request May 16, 2017
@Trott @gibfahn
benchmark: terminate child process on Windows …
0ae9583 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@Trott @MylesBorins
benchmark: terminate child process on Windows …
995423e 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@refack
test: fix flakyness with yes.exe …
ec83210 
PR-URL: nodejs#12821
Fixes: nodejs#12817
Refs: nodejs#12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott mentioned this pull request May 22, 2017
Closed
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@refack @gibfahn
test: fix flakyness with yes.exe …
7fc0946 
PR-URL: #12821
Fixes: #12817
Refs: #12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@refack @MylesBorins
test: fix flakyness with yes.exe …
739c579 
PR-URL: #12821
Fixes: #12817
Refs: #12658
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@Trott @andrew749
benchmark: terminate child process on Windows …
5efb302 
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 <refack@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the flaky-benchmark-fix branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

+2 more reviewers

@refack refack refack approved these changes

@bzoz bzoz bzoz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott @refack @gibfahn @jasnell @bzoz @MylesBorins @nodejs-github-bot