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

Uncaught exceptions thrown from promise chains don't fail tests #708

Closed
Wenzil opened this issue May 17, 2017 · 4 comments
Closed

Uncaught exceptions thrown from promise chains don't fail tests #708

Wenzil opened this issue May 17, 2017 · 4 comments
Assignees
Labels
non issue Issue is not a problem or requires changes

Comments

@Wenzil
Copy link

Wenzil commented May 17, 2017

I stumbled on a problem with how lab treats errors thrown in the context of a promise chain, but that aren't promise rejections due to being thrown from a separate function stack because of asynchronicity.

It's easier to explain with an example. Here's a failing test:

const Lab = require('lab')
const lab = exports.lab = Lab.script()

lab.test('uncaught exception from promise test', (done) => {
  Promise.resolve()
    .then(() => {
      setImmediate(() => {
        throw new Error('test error')
      })
    })
    .catch((err) => {
      // Code never reached (as expected, since the thrown error above is not a promise rejection)
      console.log('handled promise rejection!', err)
      done(err)
    })
})

lab.test('second test', (done) => {
  // Code never reached... Unless we add an 'uncaughtException' handler at the process level,
  // in which case the first case will eventually timeout, which is still undesirable
  console.log('second test started')
  done()
})

The error is simply not handled by lab and the process crashes in the first test.

If we add an 'uncaughtException' handler at the process level, the crash is prevented and the first test will eventually timeout, and then the second test will be executed. However, having to wait for the timeout is not desirable.

Lab should handle uncaught exceptions regardless of where they are thrown from.

@geek
Copy link
Member

geek commented May 17, 2017

Looks like an issue with domains.

const Domain = require('domain');

const d = Domain.create();
d.on('error', (err) => console.log(err));

d.run(() => { Promise.resolve().then(() => { setImmediate(() => { throw new Error('test error'); }) }) .catch((err) => { console.log('handled promise rejection!', err) }); });
Error: test error
    at Immediate.setImmediate (repl:1:73)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)

vs

const Domain = require('domain');

const d = Domain.create();
d.on('error', (err) => console.log(err));

d.run(() => { setImmediate(() => { throw new Error('test error'); }); });
{ Error: test error
    at Immediate.setImmediate (repl:1:42)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)
  domain:
   Domain {
     domain: null,
     _events: { error: [Function] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] },
  domainThrown: true }

@cjihrig
Copy link
Contributor

cjihrig commented May 17, 2017

My understanding is that promises + domains were broken until nodejs/node#12489. Care to give it a shot with latest Node master?

@geek
Copy link
Member

geek commented May 17, 2017

@cjihrig works!

v8.0.0-pre
~/github/node (master) $ ./node
> const Domain = require('domain');
undefined
> const d = Domain.create();
undefined
> d.on('error', (err) => console.log(err));
Domain {
  domain: null,
  _events: { error: [Function] },
  _eventsCount: 1,
  _maxListeners: undefined,
  members: [] }
> d.run(() => { Promise.resolve().then(() => { setImmediate(() => { throw new Error('test error'); }) }) .catch((err) => { console.log('handled promise rejection!', err) }); });
undefined
> { Error: test error
    at Immediate.setImmediate [as _onImmediate] (repl:1:73)
    at runCallback (timers.js:800:20)
    at tryOnImmediate (timers.js:762:5)
    at processImmediate [as _immediateCallback] (timers.js:733:5)
  domain:
   Domain {
     domain: null,
     _events: { error: [Function] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] },
  domainThrown: true }

@Wenzil the solution will be to update to the latest node release when the fix ships.

@geek geek closed this as completed May 17, 2017
@geek geek added the non issue Issue is not a problem or requires changes label May 17, 2017
@geek geek self-assigned this May 17, 2017
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
non issue Issue is not a problem or requires changes
Projects
None yet
Development

No branches or pull requests

3 participants