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

Unhandled rejection when calling `.return()` with a rejected promise from another promise constructor #1186

Closed
overlookmotel opened this Issue Aug 6, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@overlookmotel
Contributor

overlookmotel commented Aug 6, 2016

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

v3.4.1

  1. What platform and version? (For example Node.js 0.12 or Google Chrome 32)

Node v6.3.0, OS X 10.9.5

  1. Did this issue happen with earlier version of bluebird?

Issue not present on bluebird v2.10.2.


When calling .return() with a value which is a rejected promise created from a different promise constructor, an unhandled rejection occurs.

var OtherPromise = Promise.getNewLibraryCopy();

var p = OtherPromise.reject( new Error('foo') );
Promise.resolve()
    .return( p )
    .catch( function() {} );

There's no unhandled rejection where p is an instance of Promise rather than OtherPromise.

Where OtherPromise == global.Promise (i.e. native JS promise), an unhandled rejection also occurs.

I'm actually not sure if calling .return() with a promise is meant to be supported anyway (it's not stated so in the docs) but it seems to work fine apart from this odd edge case.

As with issue #1158, I'm it's more that this is an inconsistent behavior rather than a terrible bug. And this issue is more for reference than an urgent call for it to be fixed!

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 6, 2016

Contributor

Also same issue with .catchReturn()

Contributor

overlookmotel commented Aug 6, 2016

Also same issue with .catchReturn()

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

I'm still seeing this issue in v3.4.5.

The following example results in an unhandled rejection:

var OtherPromise = Promise.getNewLibraryCopy();

var p = OtherPromise.reject( new Error('foo') );

Promise.resolve()
    .return( p )
    .catch( function() {} );
Contributor

overlookmotel commented Aug 31, 2016

I'm still seeing this issue in v3.4.5.

The following example results in an unhandled rejection:

var OtherPromise = Promise.getNewLibraryCopy();

var p = OtherPromise.reject( new Error('foo') );

Promise.resolve()
    .return( p )
    .catch( function() {} );
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

It was reverted

Owner

petkaantonov commented Aug 31, 2016

It was reverted

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

Doesn't this cause the warning too?

var p = OtherPromise.reject( new Error('foo') );

Promise.resolve()
    .then(function() {
        return p;
    })
    .catch( function() {} );
Owner

petkaantonov commented Aug 31, 2016

Doesn't this cause the warning too?

var p = OtherPromise.reject( new Error('foo') );

Promise.resolve()
    .then(function() {
        return p;
    })
    .catch( function() {} );
@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

Yes, you're right. But by that logic, shouldn't the following also create an unhandled rejection?

var p = Promise.reject( new Error('foo') );

Promise.resolve()
    .return( p )
    .catch( function() {} );

(it doesn't)

The point I was trying to make is that there's an inconsistency between what happens when p is an instance of Promise and when it's a promise from another constructor (either another bluebird constructor, or a native JS Promise).

My guess was that suppressing the error in all cases is the desired behavior since .return() is called sync.

Contributor

overlookmotel commented Aug 31, 2016

Yes, you're right. But by that logic, shouldn't the following also create an unhandled rejection?

var p = Promise.reject( new Error('foo') );

Promise.resolve()
    .return( p )
    .catch( function() {} );

(it doesn't)

The point I was trying to make is that there's an inconsistency between what happens when p is an instance of Promise and when it's a promise from another constructor (either another bluebird constructor, or a native JS Promise).

My guess was that suppressing the error in all cases is the desired behavior since .return() is called sync.

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

Or, I suppose you could argue that the error should never be suppressed, as then an error can get swallowed in:

function MyError(message) {
    Error.call(this, message);
}
require('util').inherits(MyError, Error);

var p = Promise.reject( new Error('foo') );

Promise.resolve()
    .then( function() {
        throw new MyError();
    })
    .return( p )
    .catch( MyError, function() {} );
Contributor

overlookmotel commented Aug 31, 2016

Or, I suppose you could argue that the error should never be suppressed, as then an error can get swallowed in:

function MyError(message) {
    Error.call(this, message);
}
require('util').inherits(MyError, Error);

var p = Promise.reject( new Error('foo') );

Promise.resolve()
    .then( function() {
        throw new MyError();
    })
    .return( p )
    .catch( MyError, function() {} );
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

It's prohibitively expensive to go around reflecting whether the promise is some copy of bluebird, the check for if the promise is of the same library copy is very cheap so I didn't see downside in implementing it

Owner

petkaantonov commented Aug 31, 2016

It's prohibitively expensive to go around reflecting whether the promise is some copy of bluebird, the check for if the promise is of the same library copy is very cheap so I didn't see downside in implementing it

@overlookmotel

This comment has been minimized.

Show comment
Hide comment
@overlookmotel

overlookmotel Aug 31, 2016

Contributor

Ah I see. That makes sense. But what do you think of my example above where an error gets silently swallowed? (the 'foo' error gets ignored)

Contributor

overlookmotel commented Aug 31, 2016

Ah I see. That makes sense. But what do you think of my example above where an error gets silently swallowed? (the 'foo' error gets ignored)

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 31, 2016

Owner

yep in that case the error would be swallowed but doesn't seem like a big deal

Owner

petkaantonov commented Aug 31, 2016

yep in that case the error would be swallowed but doesn't seem like a big deal

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