test: fix flaky test-child-process-exec-timeout #12159

santigimeno · 2017-04-01T19:09:05Z

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM
to a process that is still starting up kills it with SIGKILL instead of
SIGTERM.

Refs: libuv/libuv#1226

Relevant backtrace:

=== release test-child-process-exec-timeout ===                    
Path: parallel/test-child-process-exec-timeout
assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: 'SIGKILL' === 'SIGTERM'
    at cp.exec.common.mustCall (/Users/sgimeno/node/node/test/parallel/test-child-process-exec-timeout.js:21:10)
    at /Users/sgimeno/node/node/test/common.js:461:15
    at ChildProcess.exithandler (child_process.js:253:5)
    at emitTwo (events.js:125:13)
    at ChildProcess.emit (events.js:213:7)
    at maybeClose (internal/child_process.js:893:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:219:5)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

Trott

LGTM

bnoordhuis

LGTM modulo style nit.

bnoordhuis · 2017-04-01T19:42:12Z

test/parallel/test-child-process-exec-timeout.js

Superfluous parens.

hiroppy · 2017-04-02T12:46:42Z

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

gibfahn · 2017-04-02T13:28:35Z

Stress test on master: https://ci.nodejs.org/job/node-stress-single-test/1150/nodes=osx1010/
Stress test on this PR: https://ci.nodejs.org/job/node-stress-single-test/1151/nodes=osx1010/

gibfahn · 2017-04-03T15:29:49Z

Looks like the stress test on master passed (running --repeat 1000 -J 10 times). Maybe it's triggered by load?

santigimeno · 2017-04-03T15:35:23Z

@gibfahn I was able to reproduce it locally on a Darwin Kernel Version 16.5.0 after lots of repetitions (more than 1000 for sure). I don't know exactly what version is running the CI bot, but this issue was initially noticed on a Darwin Kernel Version 16.4.0 box.

gibfahn · 2017-04-03T15:36:08Z

@santigimeno ahh yes, of course, we don't have a later box to test on. LGTM anyway.

santigimeno · 2017-04-04T08:36:55Z

Updated with @bnoordhuis suggestion. I'll land this later

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a process that is still starting up kills it with SIGKILL instead of SIGTERM. PR-URL: nodejs#12159 Refs: libuv/libuv#1226 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

santigimeno · 2017-04-04T13:09:28Z

Landed in 45c4ad5. Thanks

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a process that is still starting up kills it with SIGKILL instead of SIGTERM. PR-URL: nodejs#12159 Refs: libuv/libuv#1226 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a process that is still starting up kills it with SIGKILL instead of SIGTERM. PR-URL: #12159 Refs: libuv/libuv#1226 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a process that is still starting up kills it with SIGKILL instead of SIGTERM. PR-URL: nodejs/node#12159 Refs: libuv/libuv#1226 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

santigimeno added os Issues and PRs related to the os subsystem. test Issues and PRs related to the tests. labels Apr 1, 2017

Trott approved these changes Apr 1, 2017

View reviewed changes

bnoordhuis approved these changes Apr 1, 2017

View reviewed changes

test/parallel/test-child-process-exec-timeout.js Outdated

Copy link

Member

bnoordhuis Apr 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous parens.

mscdex added child_process Issues and PRs related to the child_process subsystem. and removed os Issues and PRs related to the os subsystem. labels Apr 1, 2017

richardlau approved these changes Apr 1, 2017

View reviewed changes

targos approved these changes Apr 2, 2017

View reviewed changes

gibfahn approved these changes Apr 2, 2017

View reviewed changes

hiroppy approved these changes Apr 2, 2017

View reviewed changes

santigimeno added the macos Issues and PRs related to the macOS platform / OSX. label Apr 2, 2017

cjihrig approved these changes Apr 3, 2017

View reviewed changes

jasnell approved these changes Apr 3, 2017

View reviewed changes

santigimeno force-pushed the fix_flakytest-child-process-exec-timeout branch from 6944160 to ea3fe7c Compare April 4, 2017 08:26

santigimeno force-pushed the fix_flakytest-child-process-exec-timeout branch from ea3fe7c to 45c4ad5 Compare April 4, 2017 13:02

santigimeno merged commit 45c4ad5 into nodejs:master Apr 4, 2017

santigimeno deleted the fix_flakytest-child-process-exec-timeout branch April 4, 2017 13:13

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the land-on-v6.x label Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017

v6.10.3 proposal #12498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

test: fix flaky test-child-process-exec-timeout #12159

test: fix flaky test-child-process-exec-timeout #12159

Uh oh!

santigimeno commented Apr 1, 2017

Uh oh!

Trott left a comment

Uh oh!

bnoordhuis left a comment

Uh oh!

bnoordhuis Apr 1, 2017

Uh oh!

hiroppy commented Apr 2, 2017

Uh oh!

gibfahn commented Apr 2, 2017 •

edited

Loading

Uh oh!

gibfahn commented Apr 3, 2017

Uh oh!

santigimeno commented Apr 3, 2017

Uh oh!

gibfahn commented Apr 3, 2017

Uh oh!

santigimeno commented Apr 4, 2017

Uh oh!

santigimeno commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

test: fix flaky test-child-process-exec-timeout #12159

test: fix flaky test-child-process-exec-timeout #12159

Uh oh!

Conversation

santigimeno commented Apr 1, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Apr 1, 2017

Choose a reason for hiding this comment

Uh oh!

hiroppy commented Apr 2, 2017

Uh oh!

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

Uh oh!

Uh oh!

gibfahn commented Apr 3, 2017

Uh oh!

santigimeno commented Apr 3, 2017

Uh oh!

gibfahn commented Apr 3, 2017

Uh oh!

santigimeno commented Apr 4, 2017

Uh oh!

santigimeno commented Apr 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gibfahn commented Apr 2, 2017 •

edited

Loading