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.

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

test: fix flaky test-vm-sigint-existing-handler #7982

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 4, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Set the SIGUSR2 handler before spawning the child process to make sure the signal is always handled.

(Basically the same as #7854)
Fixes: #7981

Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: nodejs#7854
Fixes: nodejs#7981
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 4, 2016
@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

LGTM but thinking that a quick code comment would be helpful also.

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@jasnell Done!

CI: https://ci.nodejs.org/job/node-test-commit/4406/
FreeBSD stress tests (the only platform besides AIX where I’d be pretty sure this is flaky given past experience, and idk if there’s a way to kick off AIX stress tests and if that’s even necessary):
master / This PR

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

LGTM if the AIX CI run is green

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Thank you! LGTM if CI is green.

speaking of which... @nodejs/build ... CI seems to be particularly flaky with arm and windows today.

@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

CI run failed, not sure if it was a one, off, trying master first to make sure still passes there: https://ci.nodejs.org/job/node-test-commit-aix/290/

@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Aug 4, 2016
@mhdawson
Copy link
Member

mhdawson commented Aug 4, 2016

CI re-run was ok so believe it was a fluke - https://ci.nodejs.org/job/node-test-commit-aix/291.
Also ran test 200 times with fix from this PR without repeats on AIX machine so looks good.

process.on('SIGUSR2', common.mustCall(() => {
// First kill() breaks the while(true) loop, second one invokes the real
// signal handlers.
process.kill(child.pid, 'SIGINT');
}, 3));

const child = spawn(process.execPath, [ __filename, 'child' ], {
Copy link
Contributor

Choose a reason for hiding this comment

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

Before landing, would you mind removing the spaces inside of the braces?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 5, 2016

LGTM

@addaleax
Copy link
Member Author

addaleax commented Aug 8, 2016

Landed in be73480

@addaleax addaleax closed this Aug 8, 2016
addaleax added a commit that referenced this pull request Aug 8, 2016
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: #7854
Fixes: #7981
PR-URL: #7982
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax addaleax deleted the fix-test-vm-sigint-existing-handler-flakiness branch August 8, 2016 13:00
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: #7854
Fixes: #7981
PR-URL: #7982
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants