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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 4, 2017
Merged

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

merged 1 commit into from
Apr 4, 2017

Conversation

santigimeno
Copy link
Member

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

@santigimeno santigimeno added os Issues and PRs related to the os subsystem. test Issues and PRs related to the tests. labels Apr 1, 2017
Trott
Trott approved these changes Apr 1, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

bnoordhuis
bnoordhuis approved these changes Apr 1, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo style nit.

test/parallel/test-child-process-exec-timeout.js Outdated
// 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.
if (common.isOSX)
assert.ok((err.signal === 'SIGTERM') || (err.signal === 'SIGKILL'));
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous parens.

@mscdex 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
@hiroppy
Copy link
Member

hiroppy commented Apr 2, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Apr 2, 2017 •
edited
Loading

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/

@santigimeno santigimeno added the macos Issues and PRs related to the macOS platform / OSX. label Apr 2, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 3, 2017

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

@santigimeno
Copy link
Member Author

@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
Copy link
Member

gibfahn commented Apr 3, 2017

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

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

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

@santigimeno
test: fix flaky test-child-process-exec-timeout
45c4ad5 
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 santigimeno force-pushed the fix_flakytest-child-process-exec-timeout branch from ea3fe7c to 45c4ad5 Compare April 4, 2017 13:02
@santigimeno santigimeno merged commit 45c4ad5 into nodejs:master Apr 4, 2017
@santigimeno
Copy link
Member Author

Landed in 45c4ad5. Thanks

@santigimeno santigimeno deleted the fix_flakytest-child-process-exec-timeout branch April 4, 2017 13:13
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
@santigimeno @italoacasas
test: fix flaky test-child-process-exec-timeout
b15dc95 
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]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@santigimeno @MylesBorins
test: fix flaky test-child-process-exec-timeout
1f732d3 
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]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@santigimeno @MylesBorins
test: fix flaky test-child-process-exec-timeout
7af8738 
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]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@santigimeno @andrew749
test: fix flaky test-child-process-exec-timeout
053e139 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@targos targos targos approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

@richardlau richardlau richardlau approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_process subsystem. macos Issues and PRs related to the macOS platform / OSX. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.