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

returning Promise.reject in async function results in Unhandled rejection error message #1404

Closed
comtaler opened this Issue Jun 7, 2017 · 21 comments

Comments

Projects
None yet
10 participants
@comtaler

comtaler commented Jun 7, 2017

  1. What version of bluebird is the issue happening on?
    3.5.0

  2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)
    Node.js 8.0.0 on Ubuntu

  3. Did this issue happen with earlier version of bluebird?
    Yes

When returning Promise.reject in async function, an error is printed on the console "Unhandled rejection" even the promise is caught. It doesn't print out the error if native Promise is used.

const Promise = require("bluebird");

async function WaitAsync(){
    return Promise.reject(new Error("reject"));
}

Promise.resolve().then(() => {
    return WaitAsync();
}).catch(err => {
    console.log("caught: ", err);
});
@sorccu

This comment has been minimized.

sorccu commented Jun 12, 2017

I believe I'm encountering another instance of this issue. In fact I was going to post a new issue before I saw this one. Here's my description:

This issue is a bit difficult to grasp, so I'll do my best to explain with examples. It seems that when using Promise.using() with an async function, awaiting on already rejected promises always outputs an Unhandled rejection when using Bluebird's Promise.reject(), but not when using native Promise.reject() or throw in an async function.

These code snippets are runnable directly on node 8 (tested on v8.1.0) with Bluebird v3.5.0 (and earlier).

First, let's start from a problematic example.

// Failing example 1

const Promise = require('bluebird')

Promise.using(Promise.resolve(), async () => {
  await Promise.reject(new Error('foo'))
})
.catch(() => console.log('OK'))

This snippet, when run, produces the following output:

Unhandled rejection Error: foo
    at Promise.using (<redacted>/a.js:4:24)
    at tryCatcher (<redacted>/node_modules/bluebird/js/release/util.js:16:23)
    at <redacted>/node_modules/bluebird/js/release/using.js:185:26
    at tryCatcher (<redacted>/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (<redacted>/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (<redacted>/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (<redacted>/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (<redacted>/node_modules/bluebird/js/release/promise.js:693:18)
    at Promise._fulfill (<redacted>/node_modules/bluebird/js/release/promise.js:638:18)
    at PromiseArray._resolve (<redacted>/node_modules/bluebird/js/release/promise_array.js:126:19)
    at PromiseArray._promiseFulfilled (<redacted>/node_modules/bluebird/js/release/promise_array.js:144:14)
    at Promise._settlePromise (<redacted>/node_modules/bluebird/js/release/promise.js:574:26)
    at Promise._settlePromise0 (<redacted>/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (<redacted>/node_modules/bluebird/js/release/promise.js:693:18)
    at Async._drainQueue (<redacted>/node_modules/bluebird/js/release/async.js:133:16)
    at Async._drainQueues (<redacted>/node_modules/bluebird/js/release/async.js:143:10)

OK

As you can see, we have an unhandled rejection error yet we did handle the error, and no clear way of rewriting the snippet so that the unhandled rejection would not occur.

Confusingly, if we reject by using native promises, we do not get the unhandled rejection. In this snippet, we only use .using() from Bluebird.

// Working example 1

const Bluebird = require('bluebird')

Bluebird.using(Promise.resolve(), async () => {
  await Promise.reject(new Error('foo'))
})
.catch(() => console.log('OK'))

This outputs simply:

OK

Similarly, this also prints the warning:

// Failing example 2

const Promise = require('bluebird')

const reject = () => Promise.reject(new Error('foo'))

Promise.using(Promise.resolve(), async () => {
  await reject()
})
.catch(() => console.log('OK'))
Unhandled rejection Error: foo
    at reject (<redacted>/a.js:3:37)
    at Promise.using (<redacted>/a.js:6:9)
    at tryCatcher (<redacted>/node_modules/bluebird/js/release/util.js:16:23)
    at <redacted>/node_modules/bluebird/js/release/using.js:185:26
    at tryCatcher (<redacted>/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (<redacted>/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (<redacted>/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (<redacted>/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (<redacted>/node_modules/bluebird/js/release/promise.js:693:18)
    at Promise._fulfill (<redacted>/node_modules/bluebird/js/release/promise.js:638:18)
    at PromiseArray._resolve (<redacted>/node_modules/bluebird/js/release/promise_array.js:126:19)
    at PromiseArray._promiseFulfilled (<redacted>/node_modules/bluebird/js/release/promise_array.js:144:14)
    at Promise._settlePromise (<redacted>/node_modules/bluebird/js/release/promise.js:574:26)
    at Promise._settlePromise0 (<redacted>/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (<redacted>/node_modules/bluebird/js/release/promise.js:693:18)
    at Async._drainQueue (<redacted>/node_modules/bluebird/js/release/async.js:133:16)

OK

However, this one doesn't:

// Working example 2

const Bluebird = require('bluebird')

const reject = () => Promise.reject(new Error('foo'))

Bluebird.using(Promise.resolve(), async () => {
  await reject()
})
.catch(() => console.log('OK'))
OK

And neither does this one:

// Working example 3

const Promise = require('bluebird')

const reject = async () => {
  throw new Error('foo')
}

Promise.using(Promise.resolve(), async () => {
  await reject()
})
.catch(() => console.log('OK'))
OK

With async/await translated to plain promises, the failing examples also work without issue:

// Previously failing example 1 (working w/o async)

const Promise = require('bluebird')

Promise.using(Promise.resolve(), () => {
  return Promise.reject(new Error('foo'))
})
.catch(() => console.log('OK'))
OK
// Previously failing example 2 (working w/o async)

const Promise = require('bluebird')

const reject = () => Promise.reject(new Error('foo'))

Promise.using(Promise.resolve(), reject)
.catch(() => console.log('OK'))
OK

This leads me to believe that there may perhaps be a bug in Bluebird, causing a rejected bluebird-promise returned by an async function to be handled asynchronously.

Thoughts?

@adamreisnz

This comment has been minimized.

adamreisnz commented Jul 30, 2017

I'm encountering this in my async express middleware when an await'ed promise rejects. The error is caught in the async middleware wrapper, but Bluebird still outputs the unhandled rejection error.

Using native promises in Node does not cause this problem.

Now that Node supports async/await natively, more and more people will start to use it and they will probably run into this issue as well.

@zowers

This comment has been minimized.

zowers commented Aug 2, 2017

as @medikoo says #1411 (comment)

async functions will always return instance of native Promise

so it looks like rejected bluebird Promise gets converted when returned from async function so the mechanics of tracking unhandled rejections do not work anymore for such cases

@adamreisnz

This comment has been minimized.

adamreisnz commented Aug 2, 2017

@zowers is that also the case when your app overwrites it in the global space, e.g. global.Promise = require('bluebird')?

@zowers

This comment has been minimized.

zowers commented Aug 2, 2017

@adamreisnz I believe there's no way to alter the type of Promise returned from async functions, it is always internal Promise

const SavePromise = global.Promise;
global.Promise = String;
async function f1() {
    return "ok"
}

const p = f1();
console.log( "f1 returned: ", p );
console.log( "p instanceof global.Promise: ", p instanceof global.Promise );
console.log( "p instanceof SavePromise: ", p instanceof SavePromise );
f1 returned:  Promise { 'ok' }
p instanceof global.Promise:  false
p instanceof SavePromise:  true
@adamreisnz

This comment has been minimized.

adamreisnz commented Aug 4, 2017

Currently "solving" this by ignoring unhandled rejections altogether, but really hoping for a proper fix soon;

Promise.onPossiblyUnhandledRejection = function() {};
@kjhangiani

This comment has been minimized.

kjhangiani commented Aug 4, 2017

We just got bit by this issue. We've just recently moved to node@8.2.1 and are using async/await extensively, along with bluebird.

I see @suguru03 has a PR to fix it, is that going to be merged soon?

@adamreisnz

This comment has been minimized.

adamreisnz commented Sep 3, 2017

Any movement on this issue and the PR?

@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Oct 2, 2017

The await mechanism simply isn't internally calling .then on the promise soon enough. It has inaccessible microtask queue that runs later than the microtask queue bluebird is using and because no .then is called by the time bluebird's microtask queue is flushed, unhandled rejection is generated.

@petkaantonov petkaantonov added the bug label Oct 2, 2017

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Oct 2, 2017

We can just wait a macrotask?

@petkaantonov

This comment has been minimized.

Owner

petkaantonov commented Oct 3, 2017

Yeah seems so. Changing this to setTimeout should do it

@adamreisnz

This comment has been minimized.

adamreisnz commented Oct 4, 2017

Awesome, thanks for the fix 🎉

@dieseldjango

This comment has been minimized.

dieseldjango commented Oct 5, 2017

I'm still seeing this on 3.5.1 in a hapi service I'm developing - unhandled rejection with bluebird, caught ok with native promises. I tried creating a simple example to repro the problem, but of course that worked fine. Could the setTimeout fire before that inaccessible microtask queue?

I'll try to create an example more like my actual code to give you.

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Oct 5, 2017

Actually it can very rarely fire an additional timer before a tick IIRC, can you try and reproduce?

@andrewaustin

This comment has been minimized.

andrewaustin commented Oct 20, 2017

Adding the setTimeout here seems to have broken a number of unit tests for us that use fake timers.

@AntonBazhal

This comment has been minimized.

AntonBazhal commented Dec 5, 2017

This change causes resource leak check to fail in our tests

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Dec 6, 2017

@AntonBazhal can you elaborate on the topic?

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Dec 6, 2017

@andrewaustin I fixed that from lolex's end (fake timers), are you still seeing the issue?

@AntonBazhal

This comment has been minimized.

AntonBazhal commented Dec 6, 2017

@benjamingr We use lambda-tester for testing our serverless applications (AWS Lambda). It does resource leak check with lambda-leak. Basically, it takes active handles before and after the test and compares them. This timer pops up during the comparison and is considered a potential resource leak since execution is already complete, but timer is still active.

@benjamingr

This comment has been minimized.

Collaborator

benjamingr commented Dec 6, 2017

Sounds like a bug should be filed against that library then. I'm unable to reproduce the issue.

@AntonBazhal

This comment has been minimized.

AntonBazhal commented Dec 6, 2017

Library does not expect any timers to hang around after execution is complete, which looks reasonable to me. Our tests fail not every time, rather in a random manner. Probably it depends on whether this timer event happens before or after the execution is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment