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

modified SIGINT handling #3570 #3602

Merged
merged 3 commits into from
Dec 14, 2018
Merged

modified SIGINT handling #3570 #3602

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
956dc59
Updated SIGINT handler based on comment from #3570
Jayasankar-m Dec 12, 2018
bc37f6a
Merge branch 'master' into issue3570
Jayasankar-m Dec 14, 2018
e5d4282
updated SIGINT handler - call only once #3570
Jayasankar-m Dec 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/cli/run-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,15 @@ exports.singleRun = (mocha, {files = [], exit = false} = {}) => {
mocha.files = files;
const runner = mocha.run(exit ? exitMocha : exitMochaLater);

process.on('SIGINT', () => {
process.once('SIGINT', () => {
debug('aborting runner');
runner.abort();

// This is a hack:
// Instead of `process.exit(130)`, set runner.failures to 130 (exit code for SIGINT)
// The amount of failures will be emitted as error code later
runner.failures = 130;
setImmediate(() => process.kill(process.pid, 'SIGINT'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This fires the signal recursively?

It doesn't work for me, FWIW...

Copy link
Contributor

Choose a reason for hiding this comment

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

If you read the GNU signal handling link I posted, reraising the signal is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boneskull
@plroebuck - Updated to reraise the signal once as specified in the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

@plroebuck That's fine, but nowhere does it say you're supposed to trigger your custom handler again. Which is what's happening if you debug this. ... at least on my machine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jayasankar-m has addressed this in 956dc59, but I have a suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

My wi-fi has been really spotty of late, with comments made but never posted.

  process.once('SIGINT', () => {
    debug('aborting runner');
    runner.abort();

    // This is a hack:
    // Instead of `process.exit(130)`, set runner.failures to 130 (exit code for SIGINT+128)
    // The amount of failures will be emitted as error code later
    process.exitCode = runner.failures = 130;
  });

Copy link
Contributor

@boneskull boneskull Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

alright, I apologize, this is coming back to me now.

The intent here is to cleanly exit, which it seems we just ditched with the PR.

We call runner.abort(), which simply sets a flag in the runner. Once the current test completes, the runner encounters this flag, and exits the process with an exit code reflecting the number of test failures.

This, of course, conflicts with the intent of this PR, which was to not let the currently-running test complete, and just exit.

I made this change so long ago, I'm not sure about the reasoning behind the "hack"; pretty sure that was me. Nor am I sure of the reasons behind the intent of a clean exit, but that's the intent nonetheless.

It's likely we've broken the "exit code with # of failures" feature here (which I realize you do not like), but only if we interrupt the process.

This seems OK to me, not recalling the original reasons for doing this, but also means the handler itself is useless and should be removed.

UPDATE: I didn't make this change, which is probably why I don't remember doing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it comes from #2438 by @Munter.

We're exiting with 130 to ensure the expected behavior--exiting with code 130--is retained while the handler disrupts the default behavior.

But because we're now actually invoking the default behavior, we don't need to call runner.abort() (because the process will immediately exit), and we don't need to set runner.failures (because the process will immediately exit) and we don't need to set process.exitCode if we just remove the damn SIGINT handler.

I'll send a PR to this effect.

});
};

Expand Down