modified SIGINT handling #3570

[Jayasankar-m]

Requirements

• Fix for 'Unable to terminate tests manually, e.g via Ctrl+C when running tests in a terminal.'
• refer issue 🐛 Bug: Unable to terminate long running tests #3570

Description of the Change

• Updated the SIGINT handler.
• A function property set to check if 'SIGINT' was called first time.
• if 'SIGINT" is called first time,keep the current behavior.
• invoke 'SIGINT' again after 2 seconds which will kill the current process if still running.

Applicable issues

see #3570

[boneskull]

Thanks for the help, but I think we have a fix incoming. I'll leave this open until I can confirm.

[coveralls]

Coverage increased (+0.04%) to 90.642% when pulling e5d4282 on Jayasankar-m:issue3570 into 6160a6a on mochajs:master.

[boneskull]

Thanks for the PR. I'm not sure it works, though. Please see my comments in #3570

[boneskull]

This fires the signal recursively?

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

[plroebuck]

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

[Jayasankar-m]

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

[boneskull]

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

[boneskull]

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

[plroebuck]

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

[boneskull]

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.

[boneskull]

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.

[boneskull]

LGTM. I'll add a test.