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

delegate to Node on non-Mocha unhandled rejections #4489

Merged
merged 2 commits into from
Nov 2, 2020

Conversation

boneskull
Copy link
Contributor

This is not intended as a fix for #4481, since it's possible that Mocha's behavior in v8.2.0 uncovers false positives. In other cases--depending on the use-case--they are not false positives at all, but rather annoyances that depended on the pre-v15 behavior of Node.js to only issue warnings.

This PR changes the behavior of Mocha so that it will re-emit unhandled rejections to process if they did not generate from Mocha. If the rejection generated from Mocha, then we treat it as an uncaught exception, because Mocha should not be in the business of ignoring its own unhandled rejections. The logic for detecting an "error originating from Mocha" is not exhaustive.

Once an unhandled rejection is re-emitted to process, Node decides what to do with it based on how it is configured to handle unhandled rejections (strict, warn, quiet, etc.).

Added a public method to errors module; isMochaError()

Ref: #4481

This is not intended as a _fix_ for #4481, since it's possible that Mocha's behavior in v8.2.0 uncovers false positives.  In other cases--depending on the use-case--they are not false positives at all, but rather annoyances that depended on the pre-v15 behavior of Node.js to only issue warnings.

This PR changes the behavior of Mocha so that it will re-emit unhandled rejections to `process` _if_ they did not generate from Mocha.  If the rejection generated from Mocha, then we treat it as an uncaught exception, because Mocha should not be in the business of ignoring its own unhandled rejections.  The logic for detecting an "error originating from Mocha" is not exhaustive.

Once an unhandled rejection is re-emitted to `process`, Node decides what to do with it based on how it is configured to handle unhandled rejections (strict, warn, quiet, etc.).

Added a public method to `errors` module; `isMochaError()`

Ref: #4481
@boneskull boneskull added semver-patch implementation requires increase of "patch" version number; "bug fixes" area: node.js command-line-or-Node.js-specific labels Oct 29, 2020
@boneskull boneskull self-assigned this Oct 29, 2020
@boneskull
Copy link
Contributor Author

think I left some test changes out of the commit, will fix it tomorrow.

@boneskull boneskull marked this pull request as draft October 30, 2020 17:10
@boneskull
Copy link
Contributor Author

I don't think this code is safe anyway (see weird "multiple done" issue cropping up in #4481). It's pretty clear the changes I made in v8.2.0 screwed something up w/r/t handling of promises, but I'm not sure what that is yet.

@boneskull boneskull force-pushed the boneskull/issue/4481-uncaught branch 2 times, most recently from abc4855 to 2b38221 Compare October 30, 2020 19:25
@coveralls
Copy link

coveralls commented Oct 30, 2020

Coverage Status

Coverage decreased (-0.04%) to 94.037% when pulling fc0b6cd on boneskull/issue/4481-uncaught into 7e490aa on master.

Signed-off-by: Christopher Hiller <[email protected]>
@boneskull boneskull force-pushed the boneskull/issue/4481-uncaught branch from 2b38221 to fc0b6cd Compare October 30, 2020 22:56
@boneskull boneskull marked this pull request as ready for review November 2, 2020 19:43
@boneskull
Copy link
Contributor Author

this actually addresses issues in #4481 and another issue causing erroneous "multiple done" output.

@boneskull boneskull merged commit c3ced39 into master Nov 2, 2020
@boneskull boneskull deleted the boneskull/issue/4481-uncaught branch November 2, 2020 19:47
@boneskull boneskull added this to the next milestone Nov 2, 2020
@WaylonOKC
Copy link

WaylonOKC commented Jan 6, 2021

This update causes a LOT of test failures for us in Heroku. We had to revert back to 8.2.0.

@boneskull
Copy link
Contributor Author

unless you can provide more info, I can’t help

@jonmellman
Copy link

@boneskull In my case, it seems to cause an extra listener call.

I can repro consistently using mocha 8.2.1. Here's a minimal repro case with no other dependencies. I'm on node 10.19.0.

it('should call a custom `unhandledRejection` listener once per unhandled rejection', async () => {
  // Register our listener
  let listenerCallCount = 0
  process.on('unhandledRejection', () => listenerCallCount++)

  // Force a rejection and wait for it to bubble up to the process EventEmitter
  const unhandledRejection = new Promise(resolve => process.once('unhandledRejection', () => resolve()))
  Promise.reject(new Error('Unhandled promise rejection!'))
  await unhandledRejection

  if (listenerCallCount !== 1) {
    throw new Error(`listenerCallCount was ${listenerCallCount}, expected 1`)
  }
})

Result:

Error: listenerCallCount was 2, expected 1

If I go into node_modules/mocha/lib/runner.js and comment out the process.emit('unhandledRejection', reason, promise);, my test passes.

So on 8.2.1 we get an extra listener call. On 8.2.0 the test fails with Uncaught Error: Unhandled promise rejection!. On 5.2.7, which I happen to be upgrading from, the test passes. Let me know what other info is helpful.

@boneskull
Copy link
Contributor Author

yeah, Mocha now has a listener for unhandledRejection--it needs one, or it will swallow rejections.

would recommend that you test for a number greater than 0, or test that a specific listener was called.

@jonmellman
Copy link

Thanks for the quick reply. I'm a little confused by what you mean by "swallow rejections", since commenting out the re-emit code shows that rejections still reach the user's listener. Maybe there are other listeners it's doesn't reach that I'm not aware of though.

In any case, the workarounds you provided are fine for me. Thanks for your work on mocha!

@boneskull
Copy link
Contributor Author

I mean that users who aren’t setting their own will not see anything and have no idea the rejection happened (depending on node version)

This was referenced Mar 8, 2021
@eyalroth eyalroth mentioned this pull request Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants