Skip to content
This repository has been archived by the owner. It is now read-only.

Default Unhandled Rejection Behavior #26

Closed
benjamingr opened this issue Feb 24, 2016 · 93 comments
Closed

Default Unhandled Rejection Behavior #26

benjamingr opened this issue Feb 24, 2016 · 93 comments

Comments

@benjamingr
Copy link
Member

@benjamingr benjamingr commented Feb 24, 2016

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.

@trevnorris
Copy link

@trevnorris trevnorris commented Feb 24, 2016

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

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented Feb 24, 2016

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

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 24, 2016

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.

@spion
Copy link

@spion spion commented Feb 25, 2016

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...

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Feb 25, 2016

@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.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 25, 2016

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

  1. 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.

  1. 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.
  2. 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.
  3. 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. :)

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 25, 2016

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.

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented Feb 25, 2016

Logging is better than not doing anything and backwards compatible

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Feb 25, 2016

@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.

@ljharb
Copy link
Member

@ljharb ljharb commented Feb 25, 2016

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.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Feb 25, 2016

@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.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 25, 2016

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..):

  1. If a promise is GC'd and still unhandled, log the stack and exit the process.
  2. 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.
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Feb 25, 2016

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.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 25, 2016

@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. :)

@ljharb
Copy link
Member

@ljharb ljharb commented Feb 25, 2016

@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.

@chrisdickinson
Copy link
Contributor

@chrisdickinson chrisdickinson commented Feb 25, 2016

Catching up now — my opinion has pretty much come full circle since the start of this conversation. The TL;DR is that I think that @Fishrock123 is on the right track by making Node crash if an unhandled rejection is garbage collected.

To expand on this: it's tempting to say we should take some default action on unhandled rejection and that programs can be universally written to avoid hitting the unhandled rejection handler. It's especially tempting because it looks like it gives @nodejs/post-mortem an out — in that "promise rejects, is unhandled, we crash", at first glance, looks like it lines up with what they need. However, we always need to wait until next tick to fire the unhandled rejection handler, because doing otherwise causes promises to exhibit confusing behavior from users' perspective — we always want to give synchronous rejections a chance to be handled on the same tick before firing unhandled rejection. This means that we always have to wait until next tick, so crashing on unhandled rejection does not help post-mortem users.

Crashing the program on unhandledRejection in the absence of a user-installed listener, as mentioned by others, introduces an artificial difference between Node Promises and browser Promises — it invalidates patterns of use that are valid in browser. This is not great for promise users, and should be avoided if we can help it.

Logging is a weird one — it seems like the most innocuous, but consider that many applications use JSON-based logging, like bole. Node logging through a separate channel means that Node will produce non-JSON output, no matter the desire of the app developer, unless they install an (otherwise meaningless) listener for the unhandledRejection event. This problem also applies to process.emit('warning').

My recommendation would be to crash on unhandled rejection GC, per @Fishrock123's plan. I'm not sure about crashing the program on exit if there are unhandled rejections, since it may be that folks are eagerly replacing a rejected promise — we'd have to investigate that. WHATWG streams may do this, IIRC.

With regards to @benjamingr's false negative example — if that were run at global scope it would never crash the program. Running it at module scope, however, would crash at some point after the module has been run. You could get it to exhibit false-negative behavior from a module by exporting the promise, requiring the module, and never handling the promise, but that seems like it would be a fairly uncommon case.

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented Feb 25, 2016

Node (and v8) already writes to stderr for plenty of reasons regardless of whether you are using JSON-based logging or not. And some of those cases are not even configurable. Having something that you can configure only default to stderr must therefore not be as big a problem as you make it out to be.

I am not sure if everyone realizes how the GC works but it is not reliable and it doesn't guarantee that an object is ever collected. I don't consider this acceptable unless you also add something that reports all unhandled rejections on process exit so that there is some guarantee.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 25, 2016

I am not sure if everyone realizes how the GC works but it is not reliable and it doesn't guarantee that an object is ever collected. I don't consider this acceptable unless you also add something that reports all unhandled rejections on process exit so that there is some guarantee.

@petkaantonov I quite agree.

I'm not sure about crashing the program on exit if there are unhandled rejections, since it may be that folks are eagerly replacing a rejected promise — we'd have to investigate that. WHATWG streams may do this, IIRC.

If there is something that would cause this to report "false positives", I'm not really sure. I'm beginning to think that promises necessitate JS vm's to do reference counting so that we can properly detect when something goes out of scope. :(

@petkaantonov
Copy link
Contributor

@petkaantonov petkaantonov commented Feb 25, 2016

If a program exits with an unhandled rejection that would have been handled had that program exited only a bit later, is it really a handled rejection? I think it depends on arbitrary philosophy. I personally just avoid the headaches of that by a simple work around:

promise.catch(noop);

setTimeout(() => {
    promise.then(function() {

    });
}, 5000);

If promise is rejected before the timeout, nothing happens yet. When the timeout executes, the then is a separate branch and it will therefore behave exactly as if that first catch had never existed and you will get unhandled rejection. So it's not like use cases like this become impossible.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Feb 26, 2016

Update: so far I'm unable to get WeakCallbacks on promises to fire in a situation that doesn't use --expose-gc. Unsure why, but I sure wish we had reference counting.

@trevnorris
Copy link

@trevnorris trevnorris commented Feb 26, 2016

Here's a test case I wrote up as a native module to demonstrate that rejected handles don't trigger the weak callback: nodejs/node#5292 (comment)

@chrishiestand
Copy link

@chrishiestand chrishiestand commented Mar 20, 2016

As a node user (not a contributor) who's been using async/await (via babel) for about 6 months, I am a big proponent of node crashing on unhandled promise rejection but I have no strong opinions on the mechanism. This would match the current behavior of an uncaught error, which feels like the semantic equivalent of not catching a rejected promise. Just like any other error, a rejected promise would need a try/catch or a .catch() to prevent a node crash. And this reinforces a current (best?) practice: catch when you don't want your program to crash - do not catch if you prefer your program bomb out with a stack trace (or if you want the calling functions to implement any catches).

I think backwards compatibility should not be an important factor here - major version releases are for (useful) breaking changes. This is a very worthwhile breaking change, imo. Plenty of warning via communication and documentation will help prepare users.

@williamkapke
Copy link
Member

@williamkapke williamkapke commented Mar 20, 2016

Please,please,please do not change the existing behavior of the current native Promises. Letting rejections be swallowed is a feature- not a bug. I have a TON of production code that leverages this intentionally. Think: Fire & Forget code... there is a genuine purpose for it.

YES- I have missed a catch where there should have been one.
YES- It has frustrated me.
... but it isn't Node's fault and it isn't Promise's fault. It is MY fault! At best, I guess it would be nice if I could turn on a flag to have them logged to help me find where I missed it. But I wouldn't want that logging spit out in the middle of my bunyan logs in production.

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

IMHO, These complaints seem equivalent to the complaints about this not being the this that was expected.

@aoberoi
Copy link
Member

@aoberoi aoberoi commented Mar 20, 2016

Is a solution that is exactly like the browsers (specifically: make hooks available for warn and un-warn, and when those hooks aren't used, warn and un-warn by writing to stderr) not viable for some reason?

i think exiting the process would be a little too extreme given @ljharb's comment about intended semantics of Promise. there are situations where not handling is exactly what a programmer may have intended because she knows how she wants to handle it later. that technique should not be "taxed" by having to structure the handlers in some specific way in node.

if you want control of the stderr output (such as needing it to be JSON formatted), attach the optional handlers and do what you want with the error. i would rather pay that "tax" than losing on explicit warnings as my default, or forcing the techniques such as above to be ruled out and superficially make my JavaScript not portable to node/browser runtimes.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Apr 4, 2016

If you known a way of clearly letting users know that they have an error that won't impossibly get lost in logs that works cross platform, other than exiting, please let me know. I don't know of such a thing.

@jrop
Copy link

@jrop jrop commented Apr 4, 2016

@Fishrock123 I haven't tested this, but is this what you're looking for?: loud-rejection.

@yosuke-furukawa
Copy link
Member

@yosuke-furukawa yosuke-furukawa commented Apr 21, 2016

I would like to progress this discussion. Current behavior is really confusing for Node and Promise users.

My first impression, I just add flags option like --throw-error-unhandled-rejection / --trace-error-unhandled-rejection, and if --throw-error-unhandled-rejection is on, node throws fatal error, if --trace-error-unhandled-rejection is on, node outputs the error to stderr.

Here is the example(currently i have not written --trace-error-unhandled-rejection option yet).

https://github.com/nodejs/node/compare/master...yosuke-furukawa:feature/promise/throw-error-unhandled-rejection?expand=1

I don't read the full discussion in this thread. So my opinion may be irrelevant, but I would like to settle down this discussion until v6 released.

IMHO, if code throws unhandledRejection, the code might have a bug. I would like to check the bug as soon as possible.
I agree with we should not change default behavior. But under the flags, we can change the behavior.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Apr 21, 2016

@yosuke-furukawa you can change the default behavior by doing:

process.on("unhandledRejection", (e) => {
    // handle error
});

Although personally I'm strongly against not having a saner default like logging.

@yosuke-furukawa
Copy link
Member

@yosuke-furukawa yosuke-furukawa commented Apr 21, 2016

@benjamingr

Yes. But I don't want to add the unhandledRejection listeners on every our products and tests. I need a saner behavior in Node core.

Although personally I'm strongly against not having a saner default like logging.

Agree, but saner default definition is quite difficult. everyone has different context. so I would like to add flags and not change default behavior.

@leodutra
Copy link

@leodutra leodutra commented Nov 2, 2016

This would make Node and JS in Node context way more powerful than Nashorn and other pure engines and put it side by side with Akka and OTP usage.

Freeing the Event Loop from this responsibility is almost a natural choice.

Maybe we'll have to rethink errors (and I don't really see a hard change on this). But at least, I think community should consider exploring more the libuv capabilities or we will have a halved awesome potential forever.

Promises should be streamed and async await should hide a streamed resolution. This is not my idea... C# and others do it.

Please, please, please think in it.

@isiahmeadows
Copy link

@isiahmeadows isiahmeadows commented Nov 2, 2016

I invite you also to check out the es-discuss mailing list. It's the best
place to talk about those kinds of things. Speaking of which, parallelism
itself has come up recently (past week or so). So do check it out.

https://esdiscuss.org/
https://mail.mozilla.org/listinfo/es-discuss

On Wed, Nov 2, 2016, 08:02 Leonardo Dutra notifications@github.com wrote:

This would make Node and JS in Node context way more powerful than Nashorn
and other pure engines and put it side by side with Akka and OTP usage.

Freeing the Event Loop from this responsibility is almost a natural choice.

Maybe we'll have to rethink errors (and I don't really see a hard change on
this). But at least, I think community should consider exploring more the
libuv capabilities or we will have a halved awesome potential forever.

Promises should be streamed and async await should hide a streamed
resolution. This is not my idea... C# and others do it.

Please, please, please think in it.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AERrBGis_-zWImm7q6oBfNYq_6TZwK8Dks5q6HvcgaJpZM4HiIA7
.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Nov 2, 2016

Great. If we gonna distort defaults for better quality of life... I'll strike once more: single threaded JS is old browser bullshit and makes JS a less competitive language.

And again, there are efforts to parallelize JS where this sort of input might be appropriate although to be completely fair I would probably try to approach those bodies with a less argumentative tone. This is about bettering the life quality of developers - let's stay positive. I'll repeat my recommendation for esdiscuss.

All async processment should run in a libuv thread. It's almost absurd we don't even try to see the benefits... Parallel is better than concurrent.

Async processing does run in a libuv threadpool and notifications are passed to node through event listeners. One of the more common misconceptions is that node is single threaded - that is certainly not the case - node just has a single JS thread. In addition, the cluster module lets you create several JS processes and communicate between those.

This would make Node and JS in Node context way more powerful than Nashorn and other pure engines and put it side by side with Akka and OTP usage.

While I appreciate both OTP and Akka I find them better than node for some things and a lot worse for others. They use a paradigms and put the emphasis in different places. Node is, like most software a result of many tradeoffs. For a lot of what I do it's fantastic, for a lot I wouldn't consider it.

Maybe we'll have to rethink errors (and I don't really see a hard change on this). But at least, I think community should consider exploring more the libuv capabilities or we will have a halved awesome potential forever.

Consider looking at the libuv.js project which exposes a much lower level API. I'm not sure what's the current status is but the great thing is that everything is done in the open and you're welcome to explore possibilities of changing how node works - I'll gladly take.

As for errors - I definitely think we need to rethink errors. Easier said than done :)

Promises should be streamed and async await should hide a streamed resolution. This is not my idea... C# and others do it.

I'm not sure what that means but I'm very intimate with how async/await is implemented in C# and it's pretty similar (sans things like synchronization context, and C#.next efforts to replace GetAwaiter with something nicer).

There are two proposals for streaming at the moment - async iterators and observables. Just to repeat, this is not the place to discuss these things - esdiscuss and GH issues on https://github.com/tc39/proposal-observable and https://github.com/tc39/proposal-async-iteration are.

It sounds like you sound passionate about this - by all means there are a lot of ways to contribute to Node, JavaScript and the ecosystem. Should you ever find yourself "stuck" ping me - there is a lot more work to go around than people able to spend time and do it.


On a less positive note - whenever you comment here it pings 22 people via email. So this is my last reply here that's not directly about the default.

Fishrock123 added a commit to Fishrock123/node that referenced this issue Feb 10, 2017
Fishrock123 added a commit to Fishrock123/node that referenced this issue Apr 28, 2017
Fishrock123 added a commit to Fishrock123/node that referenced this issue Apr 28, 2017
BridgeAR added a commit to BridgeAR/node that referenced this issue Sep 26, 2017
src: use std::map for the promise reject map

Refs: nodejs#5292
Refs: nodejs/promises#26
Refs: nodejs#6355
Refs: nodejs#6375
@bminer
Copy link

@bminer bminer commented Dec 4, 2017

@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?

@flying-sheep
Copy link

@flying-sheep flying-sheep commented Dec 4, 2017

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.

@ljharb
Copy link
Member

@ljharb ljharb commented Dec 4, 2017

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

@bminer
Copy link

@bminer bminer commented Dec 4, 2017

@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.

@jrop
Copy link

@jrop jrop commented Dec 4, 2017

@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.

@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Dec 4, 2017

@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
});
@benjamingr
Copy link
Member Author

@benjamingr benjamingr commented Dec 4, 2017

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>.

@isiahmeadows
Copy link

@isiahmeadows isiahmeadows commented Dec 5, 2017

@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).

@spion
Copy link

@spion spion commented Dec 5, 2017

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!

@leodutra
Copy link

@leodutra leodutra commented Dec 5, 2017

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.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.