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

error domain's don't always capture exceptions thrown from onPossiblyUnhandledRejection #148

Closed
iarna opened this issue Mar 17, 2014 · 21 comments

Comments

@iarna
Copy link
Contributor

@iarna iarna commented Mar 17, 2014

Specifically, the following works:

"use strict";
var Promise = require('bluebird');             
Promise.onPossiblyUnhandledRejection(function(error,promise) { throw error });

require('domain').create()
  .on('error', function(E){ console.log("ok trapped error "+E.message) })
  .run(function() {
      var P = new Promise(function(resolve,reject){ reject(new Error('boom')) });
  });

Promise.fulfilled(23).then(function(V){ console.log("#",V) });

The above outputs the following, as expected:

#23
ok trapped error boom

However, if I rearrange the promises it fails:

"use strict";
var Promise = require('bluebird');             
Promise.onPossiblyUnhandledRejection(function(error,promise) { throw error });

Promise.fulfilled(23).then(function(V){ console.log("#",V) });

require('domain').create()
  .on('error', function(E){ console.log("ok trapped error "+E.message) })
  .run(function() {
      var P = new Promise(function(resolve,reject){ reject(new Error('boom')) });
  });

And outputs:

#23

bluebird/js/main/async.js:79
            throw res.e;
                     ^
Error: boom
    at bluebird/domain-failure.js:10:60
    at new Promise (bluebird/js/main/promise.js:88:37)
    at bluebird/domain-blue.js:10:15
    at b (domain.js:183:18)
    at Domain.run (domain.js:123:23)
    at Object.<anonymous> (bluebird/domain-failure.js:9:4)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
From previous event:
    at new Promise (bluebird/js/main/promise.js:88:37)

Relates to #124

@iarna
Copy link
Contributor Author

@iarna iarna commented Mar 17, 2014

It may also be helpful to note that if I change:
Promise.fulfilled(23).then(function(V){ console.log("#",V) });
To:
Promise.fulfilled(23);
Then it goes back to working. Something about adding the chaining makes it fail to maintain the error domain.

@iarna iarna changed the title error domain's don't always work error domain's don't always capture exceptions thrown from onPossiblyUnhandledRejection Mar 17, 2014
@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Mar 17, 2014

Not saying anything about the bug but...

May I ask why you're using domains there?
Promises have a strong throw safety guarantee.

@iarna
Copy link
Contributor Author

@iarna iarna commented Mar 17, 2014

I discovered this while writing a test for another module. I think you'll find that while error domains have limited general usefulness, they're often required in test suites when one is testing normally fatal scenarios.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Mar 18, 2014

In testing scenarios you can use the good old process.onUncaughtException. Using unstable modules like domains to test your code is not a good idea:

Calling require("domain") switches out the implementation of process.nextTick, so if you have already called .then etc somewhere which caused a nextTick call, then something like this will happen.

The fix in your code is to call require("domain") at the top:

"use strict";
var Promise = require('bluebird');
var domain = require("domain");             
Promise.onPossiblyUnhandledRejection(function(error,promise) { throw error });

Promise.fulfilled(23).then(function(V){ console.log("#",V) });

domain.create()
  .on('error', function(E){ console.log("ok trapped error "+E.message) })
  .run(function() {
      var P = new Promise(function(resolve,reject){ reject(new Error('boom')) });
  });
@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Mar 18, 2014

A warning should probably be added somewhere about using domains (or better
yet avoiding it) :)

On Tue, Mar 18, 2014 at 10:08 AM, Petka Antonov notifications@github.comwrote:

In testing scenarios you can use the good old process.onUncaughtException.
Using unstable modules like domains to test your code is not a good idea:

Calling require("domain") switches out the implementation of
process.nextTick, so if you have already called .then etc somewhere which
caused a nextTick call, then something like this will happen.

The fix in your code is to call require("domain") at the top:

"use strict";var Promise = require('bluebird');var domain = require("domain"); Promise.onPossiblyUnhandledRejection(function(error,promise) { throw error });
Promise.fulfilled(23).then(function(V){ console.log("#",V) });
domain.create()
.on('error', function(E){ console.log("ok trapped error "+E.message) })
.run(function() {
var P = new Promise(function(resolve,reject){ reject(new Error('boom')) });
});

Reply to this email directly or view it on GitHubhttps://github.com//issues/148#issuecomment-37907825
.

@iarna
Copy link
Contributor Author

@iarna iarna commented Mar 18, 2014

WTF guys. I'm not asking for "help" with my code. I don't really care about that. I'm just reporting a bug in a feature that you currently have gone to some effort to support. If you don't intend to support domains then just document that.

I'm well aware that process.nextTick is swapped out. cough I'm the one that told YOU ALL that, a whole week ago when you'd accepted a massively more complicated patch to support domains.

I still stand by the idea that this is an error. I didn't throw in the first then block, so the fact that domains weren't loaded shouldn't enter into it. It's only because you're doing hinky things with nextTick queueing in schedule.js (edit: async.js) in the name of optimization that you're being bitten by this.

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Mar 18, 2014

@iarna I don't understand your last comment at all. No one closed the issue or said it's not going to get fixed.

I was just suggesting adding a warning about using domains with promises in general, and Petka suggested a general workaround for testing.

In the name of optimization as someone who doesn't use domains I don't want to take any performance hit for domains. As someone who uses domains - you'd like to be able to include domains after you include the library. I don't think Petka is convinced that there is no alternative but sacrifice performance or working with domains and being Petka he is usually quiet about resolving it when he hasn't made up his mind or figured something out yet.

Also, to be fair, I think the ugly hack here is on Node's side by doing this swapping out but this is besides the point.

@iarna
Copy link
Contributor Author

@iarna iarna commented Mar 18, 2014

@benjamingr "I don't understand your last comment at all. "

Let me rephrase then:

When people offer "work arounds" or suggest that documenting arbitrary limitations that are likely to bite real world users, this sounds to me as a prelude to saying "works as documented" rather then actually either fixing the code or declaring the feature entirely unavailable.

What I'm advocating is either: 1) Documenting the library as not supporting domains or 2) fixing the bug. I do not have a preference for either. Please stop assuming that I do.

@benjamingr "As someone who uses domains - you'd like to be able to include domains after you include the library."

No, really, see, that's where you go wrong. I'm not someone who actually uses bluebird with domains. I'm just someone who found the bug. The test suite I found the problem in, was that of another promise library that I was running bluebird through as a curiosity.

And to be fair: I totally agree that Node's swapping that out is heinous. And it turns out that 0.11's "fix" for that solves the problem with folks who take a copy of process.nextTick, however this issue is not fixed by that.

I think I have a solution for this bug, but it requires a bit more testing. And yes, I'll compare benchmarks between versions too, to ensure that it does not measurably impact the speed of the library. The speed is absolutely the draw of it– it's astonishingly fast.

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Mar 18, 2014

Fair enough, that clarified it.

Fwiw documenting the behavior as good or working around it was not suggested as a fix - I suggested a general warning about domains ( like a "do I need domains with promises?" section). I'll try to be clearer suggesting docs in the future :)

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Mar 18, 2014

Fixed in 134c396

@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 17, 2016

promises don't catch this TMK

aPromise().then(function(){
   setImmediate(function(){
       throw new Error("Won't get caught by promise");
    });
}).catch(function(err){
 // no
});

that's why we need domains, or domain-like functionality

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 17, 2016

@ORESoftware that's because you've used setImmediate. In order to enjoy the throw-safety of promises you need to promisify all your APIs. If that setImmediate becomes a Promise.delay you can throw in it and things will work like you expect.

Also see #51

Much like anyone can call process.abort in their code even if you use domains - there are always assumptions about how the code is used.

@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 17, 2016

I hear from @benjamingr that process.on('unhandledRejection') can take care of this? or no?

thanks for response - the reason why I can't "promisify all my APIs" is because I am trying to write a test framework and I can make no assumptions about the code that the users of my library will throw into the tests, unfortunately..I do look forward to the days of try/catch around async/await, but those aren't quite here yet

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 17, 2016

@ORESoftware I am @benjamingr :)

unhandledRejection would catch cases where a Promise rejected and no one handled it in a reasonable amount of time. If you throw inside a setImmediate all bets are off - it creates really confusing cases and it's part of why domains are deprecated.

aPromise().then(function(){
   return Promise.delay(0).then(function(){
       throw new Error("Won't get caught by promise");
    });
}).catch(function(err){
 // yes
});
@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 17, 2016

hmmmm, but Promises don't allow us to create (pseudo) async code...setTimeout, setImmediate, process.nextTick are what we need to do that, promises don't offer a substitute for those..so? We need process.nextTick and setImmediate, and we damn well may need them inside a promise chain.

also, as I mentioned, I am writing a test library, and I can't control the input (users will write code that may have setImmedate inside a promise etc). I believe you are very smart and I wish you would quit knocking domains until someone finds a reasonable alternative ;)

ok just kidding, perhaps promise.delay does act as a substitute, I just don't know enough about promises then...

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Feb 17, 2016

you can wrap setimmediate as promise.immediate same way as delay wraps settimeout

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Feb 17, 2016

@ORESoftware they most certainly do.

nextTick

Promise.resolve().then(() => {
   // this code always runs in the nextTick, promises guard against Zalgo by 
   // always running the code when only platform code remained
});

setTimeout

Promise.delay(100).then(() => {
   // after 100ms, uses timer internally
});
@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 17, 2016

i see, hummity hum hums

@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 18, 2016

what is Zalgo btw? Is that a real thing

@ORESoftware
Copy link

@ORESoftware ORESoftware commented Feb 19, 2016

Gotcha thanks, even after reading Isaac's post I am still not sure what
Zalgo is but sounds a bit like Pandora's Boxxx
On Feb 18, 2016 12:17 PM, "Benjamin Gruenbaum" notifications@github.com
wrote:

@ORESoftware https://github.com/ORESoftware
http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony


Reply to this email directly or view it on GitHub
#148 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.