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

.error() catches all errors from external libs #58

Closed
spion opened this Issue Dec 30, 2013 · 8 comments

Comments

Projects
None yet
5 participants
@spion
Collaborator

spion commented Dec 30, 2013

I mentioned this on IRC, but I'm also going to put it here, since I think its probably not possible to automatically deal with this one :(

var BB = require('bluebird');
var BB2 = require('bluebird/js/main/promise')(); 
BB.cast(1).then(function() { 
    return BB2.cast(1).then(function() { 
        return makes.reference.errors(); 
    }); 
}).error(function(e) { 
    console.log("This should not execute", e); 
});

This also occurs if I have a node module that depends on bluebird which I'm developing in parallel with the app that depends on it. For example, while developing anydb-sql and doxbee (which depends on anydb-sql), I can't use .error() in doxbee because that might also catch programmer errors within anydb-sql.

It gets slightly worse :( If I run npm dedupe (which is often done before browserifying or before pushing node_modules to production) the behavior of my program will change -- since now both modules will depend on the same instance of bluebird.

Anyway, in general, the assumption that an external promises-based module never has bugs is just too big -- too often this is not the case.

I realize that the situation with error types coming from most promise-based libraries is not so good, but I don't think automatic rejection marking is going to solve it -- so far it seems to be making some things worse :(

The easiest way out of this one is for promise users to settle on a flag, e.g. SomeError.prototype.isPromiseRejection = true, and for error() to all errors that have it. Then people can use create-error to make rejections

Automatic rejection marking should be possible, yes -- but opt-in, maybe via a method that converts the library in a similar manner that .promisify does that for callbacks.

What do you think?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 30, 2013

Owner

Yes .error() is now broken because of course the external promise libraries are always going to call the reject callback explicitly.

Owner

petkaantonov commented Dec 30, 2013

Yes .error() is now broken because of course the external promise libraries are always going to call the reject callback explicitly.

@Raynos

This comment has been minimized.

Show comment
Hide comment
@Raynos

Raynos Dec 30, 2013

Why does this fail? BB2.then shouldn't call reject.

Is it because BB.then calls reject if a rejected promise is returned ?

Raynos commented Dec 30, 2013

Why does this fail? BB2.then shouldn't call reject.

Is it because BB.then calls reject if a rejected promise is returned ?

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Dec 30, 2013

Collaborator

@Raynos yeah I think the thing is that when assimilating an untrusted thenable, its impossible to differentiate between thrown errors (possibly even bugs) and rejections from BB.

What could be useful? To agree on a flag/convention that errors which have the flag isPromiseRejection = true will be catched by .error() and then tgriesser's create-error can be used to easily make and throw such errors. (or simply Bluebird.RejectionError)

Basically it comes down to this -- if library authors of libraries based on promises don't make a distinction, the distinction cannot be made. I think the best you can do in that case is to filter-out TypeError, ReferenceError, RangeError as programmer errors and attach .isPromiseRejection = true to every other kind of error (this would likely be better if opt-in and... I guess it could be done with functions like rejectify/rejectifyAll over misbehaving promise-based libs -- or is that a horrible name? :D)

Collaborator

spion commented Dec 30, 2013

@Raynos yeah I think the thing is that when assimilating an untrusted thenable, its impossible to differentiate between thrown errors (possibly even bugs) and rejections from BB.

What could be useful? To agree on a flag/convention that errors which have the flag isPromiseRejection = true will be catched by .error() and then tgriesser's create-error can be used to easily make and throw such errors. (or simply Bluebird.RejectionError)

Basically it comes down to this -- if library authors of libraries based on promises don't make a distinction, the distinction cannot be made. I think the best you can do in that case is to filter-out TypeError, ReferenceError, RangeError as programmer errors and attach .isPromiseRejection = true to every other kind of error (this would likely be better if opt-in and... I guess it could be done with functions like rejectify/rejectifyAll over misbehaving promise-based libs -- or is that a horrible name? :D)

@Raynos

This comment has been minimized.

Show comment
Hide comment
@Raynos

Raynos Dec 30, 2013

@spion I don't understand how it's impossible. You mean a rejected thenable is impossible to differentiate from being rejected by reject(err) or throw err ?

Raynos commented Dec 30, 2013

@spion I don't understand how it's impossible. You mean a rejected thenable is impossible to differentiate from being rejected by reject(err) or throw err ?

@tgriesser

This comment has been minimized.

Show comment
Hide comment
@tgriesser

tgriesser Dec 30, 2013

Contributor

@petkaantonov is it necessary that the same Error objects be used (with the global freeze) on all copies of the lib rather than providing them per-instance?

Contributor

tgriesser commented Dec 30, 2013

@petkaantonov is it necessary that the same Error objects be used (with the global freeze) on all copies of the lib rather than providing them per-instance?

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Dec 30, 2013

Collaborator

@Raynos yes, and is also impossible to differentiate either from typo errors :|

And as we discussed it wouldn't matter even if reject(err) were easy to differentiate from throw, as most authors utilizing promises would simply throw their errors.

Collaborator

spion commented Dec 30, 2013

@Raynos yes, and is also impossible to differentiate either from typo errors :|

And as we discussed it wouldn't matter even if reject(err) were easy to differentiate from throw, as most authors utilizing promises would simply throw their errors.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 30, 2013

Owner

@Raynos When assimilating thenable it will always call the reject callback, it will never throw, otherwise it wouldn't be a promise library. The reference error was already caught and turned into rejection in the other library long before even calling .cast.

Owner

petkaantonov commented Dec 30, 2013

@Raynos When assimilating thenable it will always call the reject callback, it will never throw, otherwise it wouldn't be a promise library. The reference error was already caught and turned into rejection in the other library long before even calling .cast.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Jan 5, 2014

Collaborator

For the record I'd just like to add that the behavior @spion describes is exactly what I expect when assimilating thenables from other libraries - and is in my opinion the expected behavior.

That said, I understand why it's not desirable and agree that it should be fixed.


What about giving all assimilated rejections a base type of AssimilatedRejectionError and being able to catch that explicitly? That seems to both address the issue and not change current behavior.

var BB = require('bluebird');
var BB2 = require('bluebird/js/main/promise')(); 
BB.cast(1).then(function() { 
    return BB2.cast(1).then(function() { 
        return makes.reference.errors(); 
    }); 
}).catch(BB.AssimilatedRejectionError,function(e){
     // handle whatever
}).error(function(e) { 
    console.log("This should not execute", e); 
});

We can also introduce a helper for this:

var BB = require('bluebird');
var BB2 = require('bluebird/js/main/promise')(); 
BB.cast(1).assimilateAsThrow().then(function() { 
    return BB2.cast(1).then(function() { 
        return makes.reference.errors(); 
    }); 
}).error(function(e) { 
    console.log("This should not execute", e); 
});
Collaborator

benjamingr commented Jan 5, 2014

For the record I'd just like to add that the behavior @spion describes is exactly what I expect when assimilating thenables from other libraries - and is in my opinion the expected behavior.

That said, I understand why it's not desirable and agree that it should be fixed.


What about giving all assimilated rejections a base type of AssimilatedRejectionError and being able to catch that explicitly? That seems to both address the issue and not change current behavior.

var BB = require('bluebird');
var BB2 = require('bluebird/js/main/promise')(); 
BB.cast(1).then(function() { 
    return BB2.cast(1).then(function() { 
        return makes.reference.errors(); 
    }); 
}).catch(BB.AssimilatedRejectionError,function(e){
     // handle whatever
}).error(function(e) { 
    console.log("This should not execute", e); 
});

We can also introduce a helper for this:

var BB = require('bluebird');
var BB2 = require('bluebird/js/main/promise')(); 
BB.cast(1).assimilateAsThrow().then(function() { 
    return BB2.cast(1).then(function() { 
        return makes.reference.errors(); 
    }); 
}).error(function(e) { 
    console.log("This should not execute", e); 
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment