Default Unhandled Rejection Behavior

@Fishrock123

Update: Agreed course of action:

The agreed on policy is:

Abort when an unhandled rejection can never be handled (a GC hook @Fishrock123 is working on).
Warn on late catch handlers since they're likely a mistake. This is easily opt-out for users with legitimate use cases for late bindings but should really help the other 99%.

This should please everyone involved since all use cases are supported and we don't introduce abortive semantics unless we're 100% sure the promise is unhandled. Libraries can even decide to opt out of the default behavior.

Continuation of nodejs/node#830

After a few months of process.on("unhandledRejection" event I'm convinced we need a saner default.

Just to be clear - this is about the default behaviour of a rejected promise with no catch handler (or then handler with a function second argument) attached. Basically - anything that fires

The different approaches

Throwing an error for process.on("uncaughtException" to deal with. - this seems impractical now since the hook has been live in the ecosystem for a while. A flag might solve this.
Causing the process to exit. - this seems impractical now since the hook has been live in the ecosystem for a while. A flag might solve this.
Do nothing by default. - what we currently do - a lot of complaints about this.
Logging the rejection to the console. - this is what most browsers do now (namely Chrome) and what bluebird and userland libraries do. I think we should explore that path.

Proposal

When an unhandledRejection event is fired if no handlers are attached the error is logged to stderr. The current behavior is confusing to users and makes it appear as if promises swallow errors. While the unhandledRejection hook has been used a lot in the wild a lot of users (even experienced node users) can forget it and get surprising behavior.

People will still be able to opt-out of it by installing any "unhandledRejection" handler.

This aligns Node's behavior with browsers and other environments as well as userland solutions.

@chrisdickinson

/cc @chrisdickinson You've been on the front lines here. Would like your summarized opinion and any conclusions.

This is reasonable, the current behavior is really confusing - especially to people who are either new to promises or are callback users that are interacting with a promise returning library

Ugh do I really need to make my argument here rather than a PR with actual code? Fine, give me a couple days or something.

Baring the post-mortem issue (which remains unfixed) and potential shared state cleanup issues, I think this is indeed the biggest problem users have with promises. They start using them, and when the first thrown error occurs there is absolutely no feedback whatsoever as to what happened.

Browsers can add all sorts of fancy mechanisms to log (and unlog) errors and keep track of currently pending promises, unhandled rejections and so on. Unfortunately by default node only has stdout and stderr.

So yes, I think the most sensible thing to do by default is to at least log a warning, but let expert users and monitoring tools add their custom unhandledRejection (and rejectionHandled) handlers to tweak this behaviour to their liking.

Theres been a split in the promise community on this issue, with people advocating any of

interactively log and un-log unhandled rejections in some sort of UI
add .done and teach .done first
log on the next tick.

Since we don't have UI and don't have built in .done...

@Fishrock123

@Fishrock123

Ugh do I really need to make my argument here rather than a PR with actual code? Fine, give me a couple days or something.

If you'd like to pursue a PR for adding the default handler that logs by all means go for it and I'll gladly help review it.

Ok never mind, I guess I feel strongly enough about it to write it now.

Do nothing by default. - what we currently do - a lot of complaints about this.

Agreed, this isn't even a solution; we should do something.

Throwing an error for process.on("uncaughtException" to deal with. - this seems impractical now since the hook has been live in the ecosystem for a while. A flag might solve this.

Causing the process to exit. - this seems impractical now since the hook has been live in the ecosystem for a while. A flag might solve this.

Logging the rejection to the console. - this is what most browsers do now (namely Chrome) and what bluebird and userland libraries do. I think we should explore that path.

These 3 seem to conflate two things: a user-hookable event (process.on('unhandledRejection')) and the default action if there are no listeners.

[1] is impractical because that means changing the event API, and I'm not sure it even is a default action, but if an error within that handler will cause a process exit, it ends up being the same as [2], except that it is also a breaking change.

[2] isn't impractical since it's not actually a change to the API, which would be more difficult because people are most likely using it already. (We can just do something if there is no listener, that's not necessarily a breaking change.)

[3] isn't very node-like. We're not a browser, and we've long decided to exit on all other unhandled errors.

As a potential (and even past) promises in node user, I'd like uncaught/unhandled errors within promises to be consistent with other errors.

Again, we've long decided to exit on unhanded errors. I'm not exactly sure since when (other than that it was very early), or what the original reasoning was (though I can make guesses). Either way it is quite deeply baked into the node ecosystem and is (from my personal experience) probably what most node developers expect. After all, you are more likely to expect something to happen everywhere if something happens at a platform level somewhere.

Why should this be different, just because it's a different API?
It's not like domains, because domains are like registering a handler for the event. Domains are not exactly a default action.

Sure, maybe we are "wrong" about unhandled errors in promises from a browser perspective, but we are "wrong" about all unhandled errors from a browser perspective (see this twitter thread). As such, It's not very compelling to me why we would be different here. We've pretty clearly seen unhandled JavaScript errors as something the host platform (in this case Node) should decide for a long time.

I would prefer it consistent. To me there is no point in trying to continue running if I wrote the program wrong, and compared to a lot of platforms Node doesn't take much to restart. Some basic process managing can probably handle this in production, and has the upside of it being easier to detect that something is wrong rather than searching through error logs. Process managing is already a node paradigm whether you think it's ridiculous or not, and you're still going to need it in case of synchronous errors.

Now, I don't think we should exit the process in the current unhandledRejection path, but rather add a hook to "throw" on GC, if there is no default handler. You can see my current work for it at nodejs/node#5292 -- but I need to fix it up so that it actually works like that. I will be making a new PR.

The only way I can see just warning on promise errors is if we do the same from all other JavaScript errors. That is pretty impractical though, and the node developer in me doesn't even like it. Plus, it would be harder to detect potential bugs in production as I outlined above. You cannot assume people write perfect programs. :)

There's also a thing here about explicitness -- many of do not find ignoring errors a light thing. We wanted to have a flag to have unhandled errors (any kind) only stop the call stack and warn, without crashing the process, that may be viable, if it is really what you wish for.

Logging is better than not doing anything and backwards compatible

@Fishrock123

@Fishrock123 thanks.

Some clarifications: As far as I'm aware the decision to abort on all errors was partially made because errors leave node in an indeterminate state. This is not the case with promise errors generally - if a promise rejects the process generally keeps functioning correctly. This is not the case with unhandled rejections though - those imply a promise chain has been broken and no one was there to recover from an error and those can definitely leave the process in indeterminate state.

The problem is unhandledRejection can in edge cases false-positive which means you abort the process for something that gets handled later.

Code can be written in a way that does not cause an intermediate unhandledRejection and a rejectionHandled and the ecosystem mostly does so - but it can be risky to impose this on everyone and is opinionated. It's a decision the node CTC does get to make but it's one it should not make lightly. I am perfectly fine with aborting on unhandled rejection if no handler is present - it would certainly help as a first step to bridge the gap with the post-mortem folk - but it's certainly something people object to and it could certainly break some programs in node.

Also - it's not like logging does not have a precedent - we have a whole warning infrastructure for things that are deprecated, or are possible programming mistakes (like event emitters having too many handlers). Between logging and doing nothing - I'd go for logging.

As for GC - it suffers from false negatives the same way the current approach suffers from false positives. I think relying on observable GC to perform something drastic like abort the process is extremely slippery slope.

I'd like to (Try to) summon some people, should they wish to participate @erights , @domenic , @kriskowal to represent the counter-stance.

There is nothing wrong with having a rejected promise with no handler, that 10 minutes later receives a rejection handler - that's why browsers have hooks to "un-warn" about unhandled rejections. Exiting the process by default is a non-starter imo because that conflicts with the actual language semantics and expected uses.

@ljharb

@ljharb well, yes theoretically there is nothing wrong with it if it does get handled 10 minutes later. The problem is that it's impossible to determine ever when the error gets handled - it's easily reducable to the halting problem

It's entirely within the power of the node CTC to decide that in node programs such promise usage (resolving after 10 seconds) would cause the process to abort by default (with an opt-out). It's definitely an opinionated take (which is why I pinged domenic, mark and kris) who IIRC object to it.

I'm +1 on logging by the way since it's easy and has an easy opt out.

Saying something conflicts with actual language semantics and deciding what expected usage is for Node will likely not yield productive discussion - some members of the node CTC feel ignored by TC39 anyway - so I suggest we focus on problems (promises resolving after 10m) rather than telling Node what it can or cannot do.

@ljharb

Some clarifications: As far as I'm aware the decision to abort on all errors was partially made because errors leave node in an indeterminate state. This is not the case with promise errors generally - if a promise rejects the process generally keeps functioning correctly. This is not the case with unhandled rejections though - those imply a promise chain has been broken and no one was there to recover from an error and those can definitely leave the process in indeterminate state.

Not sure if this is worthwhile discussing; it seems to depend on one's definition of "indeterminate state". I think it is probably good to err on the side of safety, people may not build robust programs.

The problem is unhandledRejection can in edge cases false-positive which means you abort the process for something that gets handled later. (...)

(Also re: @ljharb)
This is why we should not exit in the existing hook, but rather on GC. If GC happens and it is still unhandled there is no hope of handing it. Unfortunately, this would be easier with reference counting but JS vm's avoid that for perf reasons afaik. See below for more info.

Also - it's not like logging does not have a precedent - we have a whole warning infrastructure for things that are deprecated, or are possible programming mistakes (like event emitters having too many handlers). Between logging and doing nothing - I'd go for logging.

In theory; logging to warn is currently very inconsistent, but hopefully that will improve with Jame's warnings PR.

As for GC - it suffers from false negatives the same way the current approach suffers from false positives. I think relying on observable GC to perform something drastic like abort the process is extremely slippery slope.

Could you define "false-negative" here?

Let me clarify my plan (which I think I can get working..):

If a promise is GC'd and still unhandled, log the stack and exit the process.
If we are exiting normally, check that there are no unhandled promise rejections, since GC may not have cleaned them yet. Follow the suit of [1] if there are.

Could you define "false-negative" here?

var settings = file.readAsync("nonexisting.json").then(JSON.parse); // let's say the file doesn't exist
// rest of the code here

Since this is in global module scope - it would probably never be garbage collected.

@benjamingr

@benjamingr right, I experienced it and thought about it, which is why I'm going to do my best to ensure my [2] is possible. :)

@benjamingr

@benjamingr fwiw, in that case i was speaking as a JS developer - I would be very displeased if my node process exited because I intentionally deferred handling of an unhandled rejection.

@Fishrock123 totally agree that if it exits on GC there's no conflict with my expectations, and it'd be a good time to exit by default.

lib,src: throw on unhanded promise rejections

@benjamingr

@benjamingr - Does anyone have an update on this? I read in nodejs/node#12734 that the new decision is to exit Node when an unhandled Promise rejection occurs and there is no unhandledRejection handler.

I agree with this behavior (long debate on nodejs/node#830), and I think that crashing when a rejected Promise is garbage collected is... well... sloppy... especially when you're just trying to make <1% of Promise users (those employing "late" catch handlers) happy. It introduces the possibility of false negatives, and it may cause programs to fail more often when under memory pressure.

As mentioned numerous times, users could simply:

process.on("unhandledRejection", () => {});

to override default behavior and add catch handlers late. Just like you can do this if you want to live on the edge:

process.on("uncaughtException", () => {});

IMHO, I think it's very important for asynchronous and synchronous uncaught errors to have the same default behavior. Currently, I just do this at the top of my index.js file:

process.on("unhandledRejection", (reason) => {throw reason;});

I don't care if I need to continue that practice or not, but I certainly have my opinions formulated about unhandled Promise rejections. What's a tad frustrating for me as a Node user is that this debate has been going on for 3+ years. :( I feel like we have all expressed our opinions, and all of the facts are out there.

That said... what is the final consensus? Any idea when the decision will impact Node 8 or Node 9 users?

fail early is a good and consistent concept. i don’t see why async stuff should be treated as some kind of low priority background stuff that can be allowed to fail.

Because the language explicitly is designed to, and does, allow late failures (#26 (comment))

@ljharb

@ljharb - We've each argued our sides. I can see the use case for late catch handlers, so I understand your frustration. Still, please keep in mind that we are only discussing default behavior; no one is saying you can't have late catch bindings. After all, it's one line of code to prevent the default behavior, and more complex logic could be split into a 3rd party module. I'd be happy to argue further, but let's take it offline -- just email me.

At this point, I'd just like someone to make a decision and stick with it. :) It's pretty clear what I'd prefer, but I'll accept whatever is best for the Node community as a whole.

@bminer

@bminer
Every catch handler has the potential to be a late error handler (the executor runs synchronously before the Promise constructor returns):

let state = 'pending'
const promise = new Promise((resolve, reject) => {
  // this executor runs before anything else,
  // and since it is synchronous, the promise-state
  // is rejected before we can attach the handler to it below:
  state = 'rejected'
  reject(new Error('uh-oh'))
})
console.log(state) // 'rejected'
promise.catch(e => { // technically attached "late"
  console.error('my handler:', e)
})

This is because Promises are not true monads: instead of "lazy-loading" them (when a handler is attached via then/catch), promises are optimistically executed.

This is why I like the GC approach.

@jrop

@jrop that's not technically attached late in Node's regard, attached late means I/O already happened:

var p = Promise.reject(new Error());
p.catch(() => {}); // not late
for(var i = 0; i < 1e9; i++); // busy loop
p.catch(() => {}); // still not late
process.nextTick(() => {
  p.catch(() => {}); // still not late
});
Promise.resolve().then(() => {
  return new Promise(resolve => process.nextTick(resolve));
}).then(() => {
  p.catch(() => {}); // still not late
});
fs.readFile("foo.txt", (err, data) => {
  // this is late
});

Also:

This is because Promises are not true monads: instead of "lazy-loading" them (when a handler is attached via then/catch), promises are optimistically executed.

Promises are not "executed", promises are the results of an already existing action. Monads have nothing to do with laziness (haskell thunk laziness does, but that's orthogonal). Monads are just things that implement the monad interface (belong to the monad typeclass). The requirements are having an unary return (Promise.resolve), a type constructor (Promise) and a bind operator (Promise.then) with the signature M a -> (a -> M b) -> M b (then with the first M being this and the single function overload - Promise<a>::then(a -> Promise<b>) -> Promise<B>.

@jrop

@jrop And to add to @benjamingr's comment, what makes Promises not monadic is that you can't nest resolved ones. Specifically, the monadic laws require that Promise<Promise<T>> must be possible (and by proxy, Promise.resolve(value).then(x => Promise.resolve(x)) be equivalent to Promise.resolve(Promise.resolve(value)), but not Promise.resolve(value)), but JS doesn't allow it. Has nothing to do with execution, but the types themselves. If JS didn't auto-resolve thenables, then it'd be a little more conforming. It was a point of conflict during the Promises/A+ design discussions, but not really anymore beyond various purists (and for them, alternatives exist).

Actually the laws don't hold even if you forbid nesting

Promise.resolve(a).then(f) <equivalent to> f(a)

Immediately this is not true, since f can throw or return a non-promise.

But lets say we restrict the type of f so that it can only return promises. A more fundamental problem that cannot be solved without breaking the A+ spec still remains. The executed side effect is deliberately not the same. The left side will execute with one tick delay on the microtask queue, while on the right side the side effect executes immediately. So the side effects are not equivalent.

This breaks application of the law in equational reasoning. If we write f(a); f(b); and then substitute based on the law with Promise.resolve(a).then(f); f(b);, in the first case the side effects will start in order; in the second case, they will start in the reverse order.

Equational reasoning is the main reason that the laws exist, so I believe the above is a sufficient reason to say that the law doesn't hold, and therefore a Promise (even the variant that would allow nesting) is not a monad. In fact, its very difficult to uphold any sort of equational reasoning if there are any eagerly executed side effects present, since the order of execution is defined by the order of evaluation!

I vote for another async hook alternative, similar to process.on + existing error upstream propagation.

Considering async/await, the stream is declared async and run in a async way. Their starter should be able to try {} catch(err){} if necessary (as it is). And the process, as a "container" of operations, should be notified of it's internal events (of any kind).

Beautiful? Maybe not for some practical eyes... but essentially aligned with general concept of processes, events and "green threads".

I still think Node.js should have a thread runner lib, using Promises/events for communication (like service workers or common Java/Go threads)... but this is another discussion. Currently, It's weird to think in a Node.js Actors model without it. And if there's someone trying to implement this way, I don't really wanna see. 🤢

+: I think logging unhandled on stderr is very valid... making possible to flag for no logging.

Navigation Menu

Repository navigation

Default Unhandled Rejection Behavior #26

The different approaches

Proposal

87 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Default Unhandled Rejection Behavior #26

Description

The different approaches

Proposal

Activity

trevnorris commented on Feb 24, 2016

petkaantonov commented on Feb 24, 2016

Fishrock123 commented on Feb 24, 2016

spion commented on Feb 25, 2016

benjamingr commented on Feb 25, 2016

Fishrock123 commented on Feb 25, 2016

​

Fishrock123 commented on Feb 25, 2016

petkaantonov commented on Feb 25, 2016

benjamingr commented on Feb 25, 2016

ljharb commented on Feb 25, 2016

benjamingr commented on Feb 25, 2016

Fishrock123 commented on Feb 25, 2016

benjamingr commented on Feb 25, 2016

Fishrock123 commented on Feb 25, 2016

ljharb commented on Feb 25, 2016

87 remaining items

bminer commented on Dec 4, 2017

flying-sheep commented on Dec 4, 2017

ljharb commented on Dec 4, 2017

bminer commented on Dec 4, 2017

jrop commented on Dec 4, 2017

benjamingr commented on Dec 4, 2017

benjamingr commented on Dec 4, 2017

dead-claudia commented on Dec 5, 2017

spion commented on Dec 5, 2017

leodutra commented on Dec 5, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions