benchmark: terminate child process on Windows by Trott · Pull Request #12658 · nodejs/node

Trott · 2017-04-26T02:57:50Z

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

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 · 2017-04-26T02:58:27Z

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 · 2017-04-26T02:58:47Z

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

Trott · 2017-04-26T03:03:36Z

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

refack · 2017-04-26T03:17:38Z

benchmark/child_process/child-process-exec-stdout.js

    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}`);


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?

NM, yes.exe is a ported unix util, it's probably crazy

refack · 2017-04-26T03:22:56Z

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 · 2017-04-26T03:29:28Z

benchmark/child_process/child-process-exec-stdout.js

    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}`);


NM, yes.exe is a ported unix util, it's probably crazy

refack · 2017-04-26T03:30:07Z

Best solution would be to use something less crazy then yes

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 · 2017-04-28T23:22:33Z

Landed in 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: #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>

Fixes: nodejs#12817 Ref: nodejs#12658

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 · 2017-05-15T19:14:18Z

Should this be backported to v6.x?

Trott · 2017-05-16T02:58:45Z

Should this be backported to v6.x?

Yes assuming the relevant benchmark exists there etc.

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>

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>

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>

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>

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 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 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

refack reviewed Apr 26, 2017

View reviewed changes

refack approved these changes Apr 26, 2017

View reviewed changes

bzoz approved these changes Apr 26, 2017

View reviewed changes

jasnell approved these changes Apr 26, 2017

View reviewed changes

Trott closed this Apr 28, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

refack added a commit to refack/node that referenced this pull request May 4, 2017

test: fix flakyness with yes.exe

7e8f7c7

Fixes: nodejs#12817 Ref: nodejs#12658

refack mentioned this pull request May 4, 2017

test: fix flakyness with yes.exe #12821

Closed

2 tasks

MylesBorins added the lts-watch-v6.x label May 15, 2017

gibfahn added land-on-v6.x and removed lts-watch-v6.x labels May 16, 2017

Trott mentioned this pull request May 22, 2017

investigate flaky sequential/test-benchmark-child-process on Windows #12560

Closed

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Trott deleted the flaky-benchmark-fix branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

benchmark: terminate child process on Windows#12658

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

Trott commented Apr 26, 2017

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

refack Apr 26, 2017

Uh oh!

refack Apr 26, 2017

Uh oh!

refack commented Apr 26, 2017

Uh oh!

refack Apr 26, 2017

Uh oh!

refack commented Apr 26, 2017

Uh oh!

Trott commented Apr 28, 2017

Uh oh!

gibfahn commented May 15, 2017 •

edited

Loading

Uh oh!

Trott commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

Trott commented Apr 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

Trott commented Apr 26, 2017

Uh oh!

refack Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

refack Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 26, 2017

Uh oh!

refack Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Apr 26, 2017

Uh oh!

Trott commented Apr 28, 2017

Uh oh!

gibfahn commented May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gibfahn commented May 15, 2017 •

edited

Loading