Move removal of uncaughtException handler into end callback

[weswigham]

And only attach it initially within start. Since self.uncaught is a bound function, "removing" it immediately before adding it only prevents the same instance from leaking multiple handles - it still allows multiple runners to leak handles forever, even when those runners runs have ended (resulting in an issue akin to #4144).

Description of the Change

Essentially, it seems like mocha 8 regressed #4144, but the test added in #4147 didn't seem to catch it, I'm guessing because now the process of of attaching the handlers during .run was deferred in some fashion by another change.

[jsf-clabot]

All committers have signed the CLA.

[boneskull]

Thanks. Can you suggest (or even implement) a test case that will assert this doesn't leak (in your use case)?

[boneskull]

Looks like, at minimum, those failing tests need to be fixed (change 2 to 1, perhaps?).

[weswigham]

The failing tests would seem to indicate that run is being called multiple times before the end event is firing, which, to me, seems problematic in its own right.

[weswigham]

Anyways, I probably can't return to this for a bit (was kinda hoping the simple fix would work in CI like it did locally); it'd be best if someone else could help pick this up (ideally someone more familiar with mocha) now that I've made someone aware of the regression. You can see the listeners warning get triggered over in https://github.com/microsoft/TypeScript with a gulp runtests-parallel after a npm install if you're wondering what the real-world example hitting the regression is. Relevant runner-management code's in src/testRunner/parallel/worker.ts if you're curious - it's just a parallel test runner. What's probably unique is that we spin up a new runner for every suite we schedule, so we make a lot of them over time. Modifying the old test to trigger the warning again should be possible; I'm actually confused as to why it wasn't already and don't reeeeealy have the time to investigate further.

[boneskull]

@weswigham It's 2 because Mocha runs these tests in its own process, which already sets a uncaughtException handler (should probably have added a comment to that effect...)

We don't have any tests for instantiating multiple Runners, but maybe that's all we need to ensure that the listeners don't leak.

Please don't mind if I/we push to your branch.

[boneskull]

@mochajs/core

To summarize:

TypeScript consumes Mocha programmatically, and they instantiate multiple Runner instances. Recent changes (regressions?) have made this leak Process.uncaughtException listeners. This PR addresses the problem, but needs the broken tests fixed and new tests added.

[weswigham]

We don't have any tests for instantiating multiple Runners, but maybe that's all we need to ensure that the listeners don't leak.

Technically, there's the one from #4147, but it doesn't seem to capture this issue, likely due to some reordering of when handlers are attached internally.

[juergba]

Removing the uncaught exception handler here is incorrect, too early, IMO. There can be exceptions bubbling up after runner's end. We even have a check for this (STATE_STOPPED) in Runner#_uncaught.

[weswigham]

If a runner 'end's, then results are likely to have been reported based on that runner. If a test could error post-end, it's too late to include it in results... Simultaneously, if a test still has outstanding actions like that, the runner probably shouldn't be 'end'ing!

[boneskull]

Sounds like @weswigham won't have time to work on this, so I can pick it up unless someone beats me to it.

[boneskull]

@weswigham I think v8.2.1 corrected a similar problem... I'm going to give it a shot against typescript.

[boneskull]

ref: microsoft/TypeScript#41403 microsoft/TypeScript#41404