Node: Default Unhandled Rejection Detection Behavior

[benjamingr]

With https://github.com/iojs/io.js/pull/758/ (hopefully) soon landing to address https://github.com/iojs/io.js/issues/256 we will soon have unhandled promise rejections reported to the process via a process.on("unhandledRejection" event.

One issue that was deliberately deferred from that issue and PR is what the default behaviour of an unhandled promise rejection detected be.

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.

The different approaches

• Logging the rejection to the console.
• Throwing an error for process.on("uncaughtException" to deal with.
• Causing the process to exit.
• Do nothing by default.

There are several good arguments for each approach. I'll write down the arguments I'm aware of and will edit arguments as people make them in comments.

Logging

• No silent failure by default.
• Does not break any existing code - no "surprises". (See comment by @vkurchatkin on this)

• Been a default in several userland libraries like bluebird and users have been very satisfied with it.

  Throwing an error

• A promise rejection is the asynchronous version of a thrown exception.

• Does not ignore logical errors or leave the app in an inconsistent state.

• False positives are extremely rare - and code causing them can arguably easily be refactored to not cause them by not attaching error handlers too late.

  Process Exit

• Differentiation between uncaught exceptions and promise rejections might be useful.

  Do nothing

• Reliable promise rejection detection is non-deciable, it's possible to report an unhandled rejection that actually is handled at a later point if the catch handler is attached at a very late time.

• Does not break any existing workflows for anyone. No backwards compatibility issues for native promise users at all.
• Does not make any assumptions on how people use native promises.

---

What do people think the default behaviour should be?

[vkurchatkin]

Does not break any existing code - no "surprises".

Actually, logging to stdout (or stderr, I guess) can cause crash: https://github.com/joyent/node/issues/6612

[golyshevd]

Logging

IMHO emitting some event it is already enough logging. Direct writing to console is dirty and inconvenient to use with custom loggers, it may violate a format of application logs.
Sometimes i think about some built-in flexible logging facility (log levels, handlers, formatters, etc.) like logging in python

Throwing an error

Much more transparent way. An exception which was caught with promise but was not handled should be thrown. But it is painful breaking behaviour. Developers should promise.then(fn).then(null, noop) to explicitly ignore exceptions

Causing the process to exit.

Does thrown error will not be caught by uncaughtException event handler? Is not is the same as previous approach?
Do I understand correctly that the difference is that in one case the exception will be sent directly in the uncaughtException event handler, and in the second case the exception will be rethrown, but will still be caught in the uncaughtException handler if specified?

Do nothing

As variant to give developers choose how to handle unhandled promise rejections

[Havvy]

Of those options, I'd argue for doing nothing. The end user can figure out what they'd like as a default, and add the handling there. If you want to be really strict about catching them, you can do process.on("unhandledRejection", function (err) { throw err; }). Personally, using Bluebird, I've only seen it once, and it was a false positive.

[benjamingr]

I'd just like to emphasise that this discussion is about _defaults_ - all options will remain open to developers and they'll still be able to decide all this.

@Havvy Thanks for your thoughts - I find those statistics very different from what bluebird users have been reporting. False positives are very rare but true positives are extremely common when people have typos or pass invalid JSON around etc.

@golyshevd - just to be clear - are you for throwing or for no default?

[petkaantonov]

The problem with doing nothing is that the feature might as well not exist as it will be undiscoverable to most developers.

IMHO emitting some event it is already enough logging. Direct writing to console is dirty and inconvenient to use with custom loggers, it may violate a format of application logs.

This is exactly what the default uncaught exception handler does minus exiting the process....

[tellnes]

I would say emitting an event eg unhandledRejection and throw if there is no event handler. Similar to how uncaughtException works. Or maybe we could emit it as an uncaughtException if there is no handler for unhandledRejection.

At least, don't choose to do nothing. Errors which are silenced is a nightmare to debug.

[golyshevd]

@benjamingr

@golyshevd - just to be clear - are you for throwing or for no default?

I am for no default as emitting event as @petkaantonov already implemented. It might be simply changed to throw like

process.on('unhandledRejection', function (err) {
    throw err;
});

process.on('uncaughtException', function (err) {
   log(err);
});

[aheckmann]

-1 for throwing bc not everyone agrees. we cannot break anyone's programs and this potentially would.

+1 for logging by default. as EventEmitter by default logs a message when the number of max listeners has been exceeded but supports overriding the default behavior through setMaxListeners etc, and as uncaughtException crashes the program by default yet supports overriding that behavior by setting a listener, we could log by default here and support overriding that behavior when a user sets an unhandledRejection listener.

+1 for documenting this at the top of the Promises page in iojs.com

—
Sent from Mailbox

On Fri, Feb 13, 2015 at 6:48 AM, Dmitry [email protected] wrote:

@benjamingr

@golyshevd - just to be clear - are you for throwing or for no default?
I am for no default as emitting event as @petkaantonov already implemented. It might be simply changed to throw like

process.on('unhandledRejection', function (err) {
    throw err;
});
process.on('uncaughtException', function (err) {
   log(err);
});

---

Reply to this email directly or view it on GitHub:
https://github.com/iojs/io.js/issues/830#issuecomment-74264116

[meandmycode]

+1 for throwing personally, but perhaps I'm not understanding the current support problem. If (native) promises are new- how are people using them to any degree yet? surely this has zero impact on promise libraries. If you decide to upgrade to native promises then you deal with the behavior change. Swallowing errors by default? seems like an unbelievable discussion for such a low level platform.

[domenic]

To be clear to those thinking this is about swallowing errors "by default": not handling a promise error is the same as forgetting to check the err parameter passed to your callback, which of course we see happening all the time in the real world as well as in example code. The fact is that promises actually have a _better_ story than callback-errors here, because since promises have two callbacks (one for fulfilled, one for rejected), we can detect if you didn't handle the error, and emit an event.

So promises swallow errors "by default" to the same extent callbacks do. This is about how we can be even better.

[domenic]

... and that actually leads to a good way of addressing the question posed in the OP.

Let's say you had a magic way of detecting if someone forgot to handle the err parameter to their callback, and instead just proceeded as normal---either on purpose or because of forgetfulness. Would you like io.js to use this magic power to log to the console when that happens? Crash your program? Do nothing?

The tradeoffs are very similar, so I hope your answer to that question can help you contextualize the question posed in the OP.

[benjamingr]

Let's say you had a magic way of detecting if someone forgot to handle the err parameter to their callback

Forgot to handle the err parameter _and_ an actual scenario occurred where the err parameter was not null (that is, an error happened).

I like the comparison by the way.

[petkaantonov]

not handling a promise error is the same as forgetting to check the err parameter passed to your callback

It's not the same at all, callback errors are always/generally operational while promise rejections can be either programmer errors or operational errors.

[meandmycode]

I disagree that unhandled promise rejection is like callback error, the error callback pattern makes it very hard to accidentally forget about the error happening, as its forced as part of callback flow, you either get what you wanted or an error, promises however break this apart (for the better), plus you are assuming that the callback pattern node has somehow proved the concept for how things should be. _It works_ but its not an ideal.

Chrome is already dealing with this issue retroactively by now ensuring that unhandled rejections throw, the only reason the entire page doesn't get dismantled is because historically the default reaction to unhandled errors in browsers is to try and keep going.

[domenic]

Chrome is already dealing with this issue retroactively by now ensuring that unhandled rejections throw

This is inaccurate. Chrome logs; it does not throw.

[meandmycode]

Ah, apologies on that detail- this seemed to be the original idea suggested from what I had last seen, but you are obviously deeper down the rabbit hole :).

Was the decision not to throw/onerror in Chrome related to how people had already started to use promises? presumably by not always chaining them correctly, ie that you may return a promise that is chained in the 'next tick' instead of the current?

If so, out of interest; were there any scenarios that meant you couldn't chain in the current tick?

[petkaantonov]

If so, out of interest; were there any scenarios that meant you couldn't chain in the current tick?

If you have a container that creates a promise that is meant to be used later then you cannot chain in the current tick. However that container should just do promise.catch(() => ) so that errors are not surfaced until the promise is chained from outside the container.

In other cases you may accidentally write such code but it can be always refactored.

[benjamingr]

@domenic can you link to the relevant parts of the streams spec that requires/needs attaching error handlers asynchronously?

[jonathanong]

i believe these are serious programmer errors, so it should kill the process or be passed to uncaughtException unless you change the default behavior. i'm not a big fan of logging because if you rely on stdout or stderr, it's noisy.

[petkaantonov]

i'm not a big fan of logging because if you rely on stdout or stderr, it's noisy.

Again, this is exactly what the default uncaughtException does minus exiting the process. Are you seriously suggesting that the default should be exiting the process without writing the stack to stderr? As a side note if your stderr is noisy you should really fix the errors :D

[jonathanong]

@petkaantonov i like my stderr silent. it's not noisy at all and i'd like to keep it that way :D

i guess a better way to pharse this is that i think these unhandled errors should be passed to uncaughtException, which (basically) console.error(err.stack); process.exit(1); unless there's a listener. in this specific instance, we can make it unhandledException to differentiate these errors.

i didn't explicitly say that it should console.error(err.stack); process.exit(1); but that's what i implied. not just process.exit(1) - that would be absolutely useless.

[Raynos]

I think one of the very useful features of uncaught exceptions is the --abort-on-uncaught-exception flag.

Having an --abort-on-unhandled-rejection flag will satisfy the most diehard "crash first" fanatics (like myself).

This means I get the most important thing I want, which is a core dump with the actual stack of the exception.

[benjamingr]

:+1: for an --abort-on-unhandled-rejection flag. I like that idea.

[benjamingr]

I think we haven't really reached a consensus here - this has not generated enough interest given how new the feature is in my opinion.

Our options are:

• Bring this up for a TC meeting which means that the TC decides on this default behaviour. Given how the discussion about the hooks themselves went it did not seem like multiple TC members had used the hooks or userland promises that provide similar hooks I do not think this is an optimal way to proceed.
• Deciding not to have a default behaviour for a while - I think that giving it half a year and seeing how people use it will give us a lot of information, that said the window for introducing default behaviour is closing since it will start breaking peoples' code (assuming the behavior that will be decided on can break).

I think the most realistic option is to give this another few months and to document that default behavior will be introduced in the future. What do you think?

[meandmycode]

My 2c here are that it may already be too late to add abortive behavior in, not only from io.js usage but how developers have been taught (or lack or teaching) from existing platforms, and the fact promises first landed in an environment that does not crash on unhandled exceptions has tainted the expectation (both of developers and its implementors).

The reality is that the concept of a _maybe unhandled?_ rejection is bad, it exists to support bad/lazy programming but is also completely impossible to abstractly handle.. what am I going to do in this event callback realistically? who knows what this promise is, it may have come from 20 layers deep of dependent packages, developers will just end up having ON UNHANDLED -> IF DEBUG -> LOG

I will say that I would expect outreach to occur to stop third party libraries filling up event logs with pointless rejections, in which case you may as well have made the behavior correct anyway.

[MicahZoltu]

export class MyData {
    constructor() {
        // prefetch to provide a better user experience
        this.resultPromise = http.get('http://www.google.com');
    }
}

export class ButtonClickHandler {
    onClick(myData) {
        // display spinner in case the promise is not yet fulfilled
        showSpinner();
        myData.resultPromise.then(result => {
            // display data to user
            showResult(result);
        }, error => {
            // display error to user
            showError(error);
        });
    }
}

In the above example, assume that MyData is constructed during application startup and the web request is kicked off right away, possibly before there is a reasonable UI to render errors to the user. At some point in the future, the user clicks some button and onClick is called. At that point in time, the promise is checked for its results.

I am of the opinion that this is a perfectly reasonable use case and generally a good practice. The promise is just an object that stores a future result or failure.

Many have related errors in promises to exceptions and I believe this is where the problem stems from. A promise is more akin to an if block than an exception. You are basically saying, "once this promise is fulfilled, run one of two code paths. You are not saying, "once this promise is fulfilled, run this code path or throw an exception." If you want the exception behavior then promises are not the right answer. You want some other construct that, while similar, does not keep rejected promises around.

[vkurchatkin]

@Zoltu this is a perfectly reasonable use case and it has been discussed a lot. You can suppress global rejection handler by attaching noop handler to your promise:

export class MyData {
    constructor() {
        // prefetch to provide a better user experience
        this.resultPromise = http.get('http://www.google.com');
        this.resultPromise.catch(() => {})
    }
}

[MicahZoltu]

Ah, great news. I have been looking around the internet at the various implementations of ES6 promises and so far io.js is the first that has provided a way to override this behavior. Great work!

[petkaantonov]

Many have related errors in promises to exceptions and I believe this is where the problem stems from. A promise is more akin to an if block than an exception. You are basically saying, "once this promise is fulfilled, run one of two code paths. You are not saying, "once this promise is fulfilled, run this code path or throw an exception." If you want the exception behavior then promises are not the right answer. You want some other construct that, while similar, does not keep rejected promises around.

This is also very wrong, 50% of the entire point of promises is that errors in promises work the same as exceptions.

[MicahZoltu]

@petkaantonov Your sentiment seems to confirm that I have correctly identified the dividing line between the two camps.

I believe that async/await will better solve the problem you are trying to solve. You can write your aync code just like your sync code and have catch blocks and last chance exception handlers.

I have read the article you linked and I didn't get the same thing from it that you did.

With async/await not terribly far off, it seems prudent to let promises solve the problem of branching/chaining on success/error and await can solve the problem of anync exception handling. If promises are implemented with last chance handlers then we will end up with two solutions to the anync exception handling problem and nothing solving the other problem.

If anync were not in sight I would be more compelled to say that promises should provide last chance handlers and there is no option for the other problem, though it seems that there would be room for two different constructs.

[benjamingr]

It seems that we're forced to have no default behavior by stagnation.

[vkurchatkin]

I don't think it's a good idea :-)

[benjamingr]

@vkurchatkin ok - can you bring it up to TC agenda then :D ?

[vkurchatkin]

We need to figure out what we are proposing. console.error seems like the safest option

[benjamingr]

console.error is something I'm definitely in favour of - the problem is getting consensus.

[vkurchatkin]

@benjamingr can you submit a PR?

[benjamingr]

The problem isn't making a PR for this behavior - it's agreeing on it first - there definitely is no consensus about it - also - you're the one who said logging can cause crashes/unexpected behavior.

[benjamingr]

@vkurchatkin as in, there is literally a place left in the original PR for this https://github.com/nodejs/io.js/blob/5759722cfacf17cc79651c81801a5e03521db053/src/node.js#L490-L491

[domenic]

Yes, this definitely requires discussion and agreement.

I think the following would be acceptable:

• A default unhandled rejection handler _and_ rejection handled handler, which _log_ (not throw) paired messages. when.js does this:

Potentially unhandled rejection [1] Error: this rejection will be handled later
    at /Users/brian/Projects/cujojs/when/experiments/unhandled.js:4:8
    at tryCatchReject (/Users/brian/Projects/cujojs/when/lib/makePromise.js:806:14)
    at FulfilledHandler.when (/Users/brian/Projects/cujojs/when/lib/makePromise.js:602:9)
    at ContinuationTask.run (/Users/brian/Projects/cujojs/when/lib/makePromise.js:726:24)
    at Scheduler._drain (/Users/brian/Projects/cujojs/when/lib/scheduler.js:56:14)
    at Scheduler.drain (/Users/brian/Projects/cujojs/when/lib/scheduler.js:21:9)
    at process._tickCallback (node.js:419:13)
    at Function.Module.runMain (module.js:499:11)
    at startup (node.js:119:16)
    at node.js:906:3

... one second later ...

Handled previous rejection [1] Error: this rejection will be handled later

with the [1] being the rejection ID.

• A --abort-on-unhandled-rejection flag for throwing.
• A --ignore-unhandled-rejection flag for no default behavior.

(You could consolidate the two flags into one or name them differently; I don't have a strong opinion.)

[benjamingr]

Why do we need flags @domenic? If someone wants to bypass the default behaviour they'd just need to add a handler?

As for default behaviour - I think logging in that format is a pretty reasonable default.

[domenic]

It's a convenience. We're packaging up three potential defaults that people might commonly want to use: nothing, log, and abort. And we're allowing people to choose which one to apply through CLI flags, instead of having to modify their code.

[Raynos]

Edited: leaving comments on phone is a bad idea.

@domenic --abort-on-unhandled-rejection is iffy

You would expect semantics like abort on uncaught exception.
I.e. Reject(e) needs to throw and abort with a meaningful stack.

I want to read the core file. Explore the real stack. I do not want to see a string stack trace but a real stack where I can exactly who rejected this u handled promise.

Just throwing an error in next tick where the stack has two frames would be useless.

If we cannot implement this abort semantics then we should have a different flag (i.e. --throw-on-unhandled-rejection).
And even if we had abort I would still want a throw on unhandled flag. Abort is for production. Throwing is for tests and local development.

[trevnorris]

re: using console.error(). would probably be best to use process._rawDebug().

[trevnorris]

A --abort-on-unhandled-rejection flag for throwing.

Then I would expect --throw-on-unhandled-rejection. abort implies a core dump, which throwing is not guaranteed to do.

[domenic]

I wasn't aware of the difderence; thanks guys.

[Trott]

Conversation on this seems to have died down again. I could be wrong on the calculus, but it does seem like the longer this sits, the harder it will be to decide that the behavior should be something other than Do Nothing. Is it premature to tag this tsc-agenda in the hopes a decision can be made?

[benjamingr]

@Trott good idea! I'm definitely in favor of re-starting this discussion

[MicahZoltu]

It seems that any decision is going to upset someone. The best course of action I believe is to make it so whatever decision is made can be overridden by the user in some way, preferably on a per-application level (not per run). A global flag that my application can set during startup would be a simple example of this.

Personally, I am of the opinion that the default behavior should be exactly in line with the specification and you have to opt-in to behavior that is not part of the spec. That being said, there is value in having extended support for the most simple use of promises be the default and users leveraging promises in advanced ways can opt-out of that behavior.

[Trott]

Based on my best effort to understand the dev policy, I've tagged this with tsc-agenda in the hopes of getting some kind of decision. (Hopefully I'm not Doing It Wrong, and if I am, well, it should be easy enough for someone who knows better to untag this.)

My best guess is that, at this late date, "Do Nothing" is the obvious choice as it basically guarantees not breaking anything in the ecosystem. But I'm not opposed to logging a warning instead of doing nothing. But it does seem like every week that goes by without a decision makes it harder and harder to choose an option that isn't "Do Nothing". So it would be good to get some kind of official consensus on the way forward here.

FWIW, I'm for "Do Nothing" and close the issue (for the reason above), with "log a warning" as my second choice.

EDIT: Oh, yeah, behavior changes based on CLI flags and/or other option settings, totally awesome. But this issue is about _default_ behavior, and, IMO, I think the default option should be either do nothing or log a warning.

[thefourtheye]

@trott +1. I think issuing a one time message would be a good default behavior in this case.

[Fishrock123]

@Trott most of the TSC aren't super in tune to this issue, so we'd need some set of options to decide from. :)

[MicahZoltu]

Background reading material for the subject:
https://github.com/promises-aplus/unhandled-rejections-spec/issues/1

List of proposals with discussions on each:
https://github.com/promises-aplus/unhandled-rejections-spec/issues

Personally, I would recommend waiting until the promises spec decides and then follow suit. Unfortunately, they are as stalled out as everyone else. I believe part of the reason is because one camp explicitly wants no-change while the other camp wants a change. It is always very hard for a change camp to win against a camp that explicitly wants no-change.

Full Disclosure: I am strongly in the no-change camp.

[benjamingr]

@Fishrock123 I wouldn't mind participating in a meeting and explaining the alternatives (which are listed at the first post in this issue) and their caveats. Alternatively I'm sure @petkaantonov and @domenic would also be able to do so and understand the subject as in depth, if not more, than me.

Another thing to consider is that browsers are working on speccing this too, in retrospect not waiting for them to come to a consensus was a good call and I've personally enjoyed having the hooks a lot and they've been a "killer feature" in convincing people to migrate from Node 0.12 to io.js when talking to people.

Logging would be very valuable to users who are not aware of the hooks, this is what browsers do (although browsers don't have hooks yet). I personally crash, but I think that's "not for everyone". As this can be solved in userland no-change also makes sense and is a viable alternative.

[monsanto]

Just to add my 2c to this, I use promises extensively in production applications and in my experience registering error handlers asynchronously is a novelty. Others may have a different opinion on that, but this is mine. Given that & the fact that Good Defaults Matter:

+1 to either logging or uncaughtException
-1 on exit process, there is no advantage of doing this over uncaughtException
-1 on doing nothing, this is a terrible default. If you don't want to hear about very likely errors (at least in my exp) then explicitly turn them off.

[rvagg]

At this stage the TSC is likely to just defer to @domenic for his perspective on this I thing. Unless we can have a clear outline of what we're supposed to be discussing and agreeing on I don't see this being a very constructive TSC discussion.

My position currently (sounds opinionated but I'm trying to keep an open mind about this), and I suspect others on the TSC will be similar:

• If you're using Promises then you're choosing a paradigm where error handling is second class, that's your choice and you should be aware of this when going in. If you are not going to handle errors then you're also making a choice. This is very similar to using streams or EventEmitter and not handling 'error'.
• It's not Node's job to _fix_ that problem that exists with Promises, this is either a language-level discussion or choose a better abstraction. There's a limit to how much we can and should protect against foot-guns. Unlike streams and EventEmitter, the Promise object is not a Node-native so we can't apply the same implementation reasoning to it (i.e. we don't own Promises in the same way that we own streams and EE).
• When we merged the unhandledRejection behaviour we did it with a minimal footprint approach, leaving open the option of changing that in the future if it made sense
• There doesn't appear (from what I can see) to be an emerging agreement in any sphere (browsers, Promises implementation libraries), having Node pick a side could leave us on the wrong side of a future agreement on how it _should_ be handled.

[Fishrock123]

To me, I would expect an error that would crash with non-promises would also crash with promises, and I also suspect that the average user might also.

That being said, I think I'm with @rvagg here. Just if we aren't going to touch it we need to make sure our docs are clear about unhandledRejection existing. (They might already be?)

[rvagg]

@Fishrock123 you can't apply that reasoning directly to Promises because you don't have a clear "this error is not handled" case, it _may_ be handled at some future point and it's fairly arbitrary that we've chosen when to even trigger an unhandledRejection, why not do it 10 ticks later? why not 10 seconds later? a minute? at the end of the application??? The original PR for it had it possiblyUnhandledRejection IIRC which is technically more accurate. Therefore, if you don't know that the error is going to be unhandled, choosing a point in time to crash after an error is almost entirely arbitrary!

[domenic]

Right, I read "I would expect an error that would crash with non-promises would also crash with promises" as no crashes at all, since nothing crashes when you do fs.readFile(..., function (err, result) { /* ignore err */ }).

[Fishrock123]

The original PR for it had it possiblyUnhandledRejection IIRC which is technically more accurate. Therefore, if you don't know that the error is going to be unhandled, choosing a point in time to crash after an error is almost entirely arbitrary!

oh. Promises are complex, deferring to people who actually understand this crazy concept. :P

I guess @domenic's explanation makes sense though.

[trevnorris]

@domenic's comparison is an appropriate one. In both cases the error is made available to the user without throwing and it's up to the user to ignore it. The only difference is the scope in which the error can be retrieved.

[trevnorris]

There is another discrepancy I just thought of. Promise executors run synchronously, but also have an implicit try/catch.

In node functions that run synchronously after execution allow exceptions to propagate. The problem this presents is, AFAIK, it is impossible to differentiate between when the code in an executor threw because it misbehaved and when the reject callback is called.

I have no input on how this should be handled. Just wanted to point out that "never throws" isn't a completely valid comparison.

[meandmycode]

I think the thoughts of 'we have to do something' are probably too late. It's shipped and already does something developers are reliant on, and to a higher degree, developers already used promises way before native ones.

I can't see behavior changing here, the TC is far too ill confident of what should happen that as soon as the first bug report of the changed behavior comes in, you'll revert it back.

My experience with promises here is that unlike callback style errors we can make a pretty strong estimate that you have a bug in your app if a promise goes unhandled for a tick.

This sort of 'there may be a bug here' logic typically lives in the IDE / compiler. But as we know, node is unusual here and has no requirement for those, which may suggest the runtime handle them, but I'm cautious to say that's true as I expect runtime to be as fast as it can be.

Perhaps this comes back to JavaScript needing some good static analysis tooling.

Anyway, I digress, my point is that silent failing with a hook seems the most likely outcome.

[domenic]

To me this basically comes down to how the TSC feels about printing things to stdout/stderr without user opt in. If they think that's fine, then maybe we could add a default handler that logs (but doesn't crash or throw or anything). If they don't, we should leave things as-is. We can discuss a bit during the TSC meeting I guess.

[rvagg]

This was discussed at last week's meeting.

Action: nothing now, maybe if v8 adds a hook for when rejections get garbage collected.

i.e. when a rejection that's unhandled gets garbage collected, that would be a more ideal time to take action (of some kind) because it's much less arbitrary than what we have now and it's much easier to justify action because of the clarity that GC provides. It seems likely that V8 will include these hooks soon, according to @domenic I think.

[domenic]

Slight clarification. V8 already gives us all the hooks we need. It's on us to investigate the possibilities. It should be possible to watch for unhandled rejected promises, and add a weak callback of sorts to them that checks if they are still unhandled when GCed, entirely from the io.js codebase.

If someone wants to take on that project, there's definite TC interest.

[benjamingr]

I think relying on GC for specified behavior might be useful but taking action based on something nondeterministic such as GC is something a lot of people would object to. I suspect @erights among them.

[domenic]

If that action is logging a debugging aid there's no problem. This wouldn't be user-hookable.

[benjamingr]

If it's not user-hookable I'm perfectly fine with it, I think it would even be incredibly useful :)

[erights]

First, I am fine with this as a first step if it can only be observed by those with the privileged ability to observe the log. But we should not stop there. In general, adequately authorized user-mode code should be able to emulate, in a self contained manner, many of the things that are contributed by the platform. I realized that is vague in general because we need to examine things on a case by case basis.

For this case, http://wiki.ecmascript.org/doku.php?id=strawman:weak_references and http://wiki.ecmascript.org/doku.php?id=strawman:weak_refs , once unified, would allow this GC-dependent logging to happen by user-code that has been access to the special makeWeakRef capability. As stated on those pages, we have purposely postponed making weak refs available until we have enough experience with the module system to know what patterns are pleasant for providing some modules only to privileged parties.

So having this info available only at debugger/console/log level until then seems like a fine step.

[bminer]

My gut reaction to default behavior is to throw an unexception whenever a rejected promise is not handled and there is no "unhandledRejection" event handler. This prints a nice stack trace to stderr and terminates Node with exit code 1... this is consistent with the behavior of not using Promises.

Personally, this is what I expected as default behavior before I learned about the "unhandledRejection" and "rejectionHandled" events. Now that I know about them, it still feels like a good idea to raise an uncaught exception here. IMHO, a typo or syntax error in Promise.then should be an uncaught exception (unless explicitly handled by a "unhandledRejection" event handler).

Consider this example:

var promise = new Promise(function(resolve) {
    kjjdjf();
});

What happens in Node 4.0.0? The program silently continues. If you were a n00b with Promises (like me), the "unhandledRejection" would be ignored and your program would be in an undefined state.

How is this much different from writing: kjjdjf();??? Well, it's not encapsulated by a Promise for one! Not writing promise.catch() is much like forgetting to write try {} catch(e) {}. IMHO, throw an uncaught exception whenever there is an unhandled rejection and no registered "unhandledRejection" event handler.

[LinusU]

I just submitted my idea here: https://github.com/sindresorhus/loud-rejection/pull/4

What it does is the following; any time a promise that did not have it's failure handled is garbage collected, it throws an error.

I think that this is a great approach since it allows the following:

var p = new Promise(function (resolve, reject) {
  setTimeout(reject, 20, new Error('unicorn'))
})

setTimeout(function () {
  p.catch(function () {
    console.log('I caught you')
  })
}, 140)

But it actually catches stuff like this:

new Promise(function (resolve, reject) {
  setTimeout(reject, 20, new Error('unicorn'))
})

I would love to see this as standard behaviour. What do you guys think?

[bminer]

@LinusU - I don't see how this really pertains to unhandled rejections. In your first example where p.catch is called after reject is called, that is still an unhandled rejection at that point and is later handled by p.catch. What's to say that p.catch is ever called? What if p.catch is only called conditionally?

Anyway, I still don't understand why an unhandled rejection does not raise an exception by default... maybe someone could explain it to me???

[LinusU]

@bminer The point is that whenever p goes out of scope an exception will be raised, that way the exception will always be thrown, even if p.catch is only called conditionally.

If we raise an exception by default, the following would crash, even thought it's perfectly valid.

function SQLConnection () {
  this.connection = new Promise(function (resolve, reject) {
    setTimeout(reject, 10, new Error('Server error'))
  })
}

SQLConnection.prototype.query = function (query, cb) {
  this.connection.then(function (c) {
    // run query on c
  }, function (err) {
    cb(err)
  })
}

// ------

var c = new SQLConnection()
setTimeout(function () {
  c.query('SELECT 1', function (err) {
    console.log('The error', err)
  })
}, 600)

That's why we need this, and why we don't always raise an exception.

[MicahZoltu]

Logging at GC time is definitely better than logging on next tick or similar. However, it still makes an assumption that _everyone_ cares about rejections. Depending on whether you think about promise rejections as exceptions or error returns, this isn't a valid assumption.

// don't care about the results on this invocation, it is a best effort reporting system
httpClient.fetchAsync("http://www.example.com/marketing");

// care very much about the results on this, we have fallback behavior that runs on failure
httpClient.fetchAsync("http://www.example.com/feature").catch(error => handleError(error));

In the first case, I would get an unhandled rejection if I got an error from the endpoint. However, I don't care if it errors, it is best effort. The same goes for advertising, if I fail to get an advertisement on a page load, I probably don't care and catching the error would be incorrect behavior (other than to silence an unhandled rejection).

In the second case, not catching _is_ an error because I care about the result.

If you think of a rejected promise as an _exception_ then a log message at GC time is appropriate. If you think of rejected promises as error returns though then it isn't appropriate.

let result = httpClient.fetchAsync(...);
if (result is Error) {
    // handle error
} else {
    // process result
}

vs

try {
    let result = httpClient.fetchAsync(...);
    // process result
} catch {
    // handle error
}

[bminer]

@LinusU - Your example is perfectly valid. That is, the code works and runs properly, but it still causes an _unhandled_ rejection. The point here is to determine the default behavior for unhandled rejections. You can still write your own "unhandledRejection" event handler and do whatever you want. Better yet, you could write this.connection.catch(...) and properly handle the rejection. But, by default, I think your code should raise an exception when reject is called and the promise does not have a rejection callback by the next tick. Doing stuff at GC time can be confusing and unpredictable.

Promise rejections remind me of exceptions. When reject is called, it means that some operation failed; more than likely you want to handle this case. I'll even boldly question why you would use Promises in the first place if you aren't handling the Promise rejection case. If you REALLY don't care about the error, a simple p.catch(function nop() {}) is a good way to ignore the rejection. IMHO, this is similar to writing an empty catch(e) {} block.

Reiterating a little... Unhandled rejections feel a LOT like uncaught exceptions; they are errors that you forgot to handle (not fact, just my opinion). If an unhandled rejection occurs, you can always handle it nicely in an "unhandledRejection" event handler, or you can revise your code to add a rejection event handler to the Promise. But, by default, I am arguing that an unhandled rejection should be treated just like an uncaught exception. If an unhandled rejection occurred and there was no "unhandledRejection" event handler, an uncaught exception would be thrown, the "uncaughtException" handler would be fired, and if there wasn't one, the program would crash and print a stack trace.

Now, I understand that uncaught exceptions are a bit more serious because they can blow up an entire stack trace and leave the program in an undefined state; whereas, rejected Promises are not dangerous in this way. Still, I believe that this default behavior provides a safeguard for programmers who are new to Promises. If they want to override the default behavior and enter slightly-more-dangerous territory, they are free to add "unhandledRejection" event handlers and the like.

[benjamingr]

Now, I understand that uncaught exceptions are a bit more serious because they can blow up an entire stack trace and leave the program in an undefined state; whereas, rejected Promises are not dangerous in this way

That's not true, if you have an unhandled exception inside a then handler it will get converted to a rejection. So there is a potential for leaving the program in undefined state. Just like if you'd try/catch every piece of code.

I'm not really buying the whole "undefined state" thing for most regular user code - I think that leaving node in an undetermined state is pretty hard in most scenarios you'd use promises in in the first place :)

[LinusU]

@bminer The problem is that my code is valid, but if we started throwing on unhandled rejection, it would crash. I want to wait until the user tries to run .query, and then return the error to the user. This is why I believe that during garbage collecting is the absolute earliest moment when we can do this.

@benjamingr The idea of "undefined state" is that you cannot know if it is in a good state or not. Even if it's fine 99% of the time it's still "undefined", since you can't be sure.

[bminer]

@LinusU - I see your point. Still, why not just handle the connection Error immediately and store the Error in some property (i.e. this._connectionError)? Then, you can check for this._connectionError in the .query method and pass it to the cb. IMHO, doing it this way makes the code more readable and more clearly conveys what you're trying to do. Appending rejection handlers after the Promise could already be rejected seems sloppy to me. It's going to raise an "unhandledRejection" event, and then raise a corresponding "rejectionHandled" event later once .query is called. That's fine and all, but it seems strange.

I don't want to say "You can't code that way" or "Your code is stupid." I'm just talking about _default behavior_ of unhandled Promise rejections, which can be overwritten if you want. For example, if I got my way on this, you could overwrite my default behavior to make your code work like this: process.on("unhandledRejection", function nop() {});

Although in the same breath, to get my desired behavior, I could write:

process.on("unhandledRejection", function(reason, p) {
  if(process.listenerCount("unhandledRejection") === 1) {
    /* Throw some sort of uncaught exception here since there is no other
    unhandledRejection event handler */
    var err = new Error("Unhandled Promise rejection");
    err.reason = reason;
    err.promise = p;
    throw err;
  }
});

I dunno. This seems like more of a religious discussion than anything else. Developers have been using Promises for a long while now, and they don't want to see their code get broken. I get it... but sometimes, breaking code is a good thing?

[LinusU]

The problem with globally overriding is that it goes for the entire system, including all of the modules you include. This just seems to fragile.

Also, that was just an example, I think that there are more situations in which it takes a while to handle the exception. e.g.

async function cookMeal () {
  let fishPromise = getFish()
  let potatoPromise = getPotato()
  let saltPromise = getSalt()

  let salt = await saltPromise

  let fish = await combine(salt, await fishPromise)
  let potato = await combine(salt, await potatoPromise)

  return [fish, potato]
}

If the salt takes 1000ms to fetch, but the fish-fetching fails after 500ms. The fishPromise won't be handled until the await fishPromise.

[benjamingr]

@benjamingr The idea of "undefined state" is that you cannot know if it is in a good state or not. Even if it's fine 99% of the time it's still "undefined", since you can't be sure.

This is also the case with unhandled rejections @LinusU .

The problem with globally overriding is that it goes for the entire system, including all of the modules you include. This just seems to fragile.

The whole point of the hook is to have _one_ place for _all_ promises code that emitted an unhandled rejection to get handled. This is why libraries implement this too.

[benjamingr]

Also, that was just an example, I think that there are more situations in which it takes a while to handle the exception. e.g.

That's actually a very motivating example against a throw default. I wonder if async functions should just be allowed to finish to run before unhandled rejections get reported. Hmmm...

[bminer]

@LinusU - Hmm, very good point. if we pull the ES7 async/await stuff into the discussion, I see your point. In your example, I suppose you could write something like await Promise.all(saltPromise, fishPromise, potatoPromise). But, there might also be cases where you really want to await salePromise only and do some more processing with salt before awaiting the other promises.

In this case, I agree with @benjamingr. async functions should be allowed to finish before unhandled rejections even get reported. That should handle this case.

[MicahZoltu]

I believe that with async/await the throw should occur on the call to await, not on rejection. This is how it is done in c# and it results in very intuitive behavior. It is also part of why I don't think unhandled rejection logging should occur, because there is a much more natural way to deal with the problem once async/await lands.

[bminer]

@Zoltu - Sure. The throw _does_ occur on the call to await. The problem is that, in the meantime, you technically have an unhandled rejection. A proposed solution is to silently ignore the unhandled rejection _until_ the async function terminates and control is returned to the event loop.

This solution might be tricky, though. What if the rejected promise has global scope and is accessible to the entire codebase (unlikely but let's go with it)? In this case, Node might have to wait for ALL async functions to terminate before raising the "unhandledRejection" event.

[MicahZoltu]

TL;DR: async/await solves the unhandled rejection problem. Putting in another solution now (while waiting for async/await) will only serve to cripple promises down the road and will yield no long-term benefit.

---

Looking at C# as a case study, once async/await was developers stopped using Tasks (same as a ES Promise) directly with the exception of low-level library authors. Developers now almost exclusively interact with them via the _much_ more intuitive async/await pattern syntax. I suspect that the same will become true of ES once async/await is generally available.

Because of this, I think it is much more important to consider the desired behavior of promises at that future point in time rather than right now. At the moment, people are having to interact with promises directly which is why they are struggling with unhandled rejections. In the future, they will likely be interacting with them via async/await so the average developer won't need to care about unhandled rejections anymore (since rejections turn into throws at await time).

If we assume that the future of ES async/await will be similar to C#, then I think it is safe to assume that the people interacting with Promises directly are going to be advanced library authors, not your average web front end JavaScript developer. The argument _against_ logging unhandled rejections tend to be coming from advanced usage scenarios (i.e., a similar demographic to the library authors that will be working directly with promises) while the arguments _for_ logging unhandled rejections tend to be coming from more simplistic usage scenarios (i.e., a similar demographic to the async/await users of the future).

By putting in the unhandled rejection logging now, I fear that promises of the future (post async-await) are going to be crippled in their capability without any real long-term benefit (the benefits are short-term, while waiting for async/await to land).

[bminer]

Sorry to flip-flop here, but I'm going to change my mind again. Let's just make the default behavior for unhandled rejections throwing an uncaught exception (for the same reasons outlined above)

@LinusU - I have found a way to re-write your example to avoid unhandled promise rejections. See below:

async function cookMeal () {
  let fishPromise = getFish();
  let potatoPromise = getPotato();
  let saltPromise = getSalt();

  let saltedFishPromise = Promise.all([saltPromise, fishPromise])
    .then(function(saltAndFish) {
      // Note: `saltAndFish` is an Array of 2 elements
      return combine(...saltAndFish);
    });
  let saltedPotatoPromise = Promise.all([saltPromise, potatoPromise])
    .then(function(saltAndPotato) {
      return combine(...saltAndPotato);
    });
  return await Promise.all([saltedFishPromise, saltedPotatoPromise]);
}

Admittedly, the code is a bit longer, but it more closely represents what we're trying to do... which is to cook a meal with maximum concurrency :) Code above is not tested, so please go easy on me.

[benjamingr]

@bminer the argument was never that it's _impossible_ to write code without unhandled rejections firing. The argument was that it's _non obvious_ and in practice people are going to write code that does report unhandled rejections.

[bminer]

@benjamingr - Well, IMHO, you can choose to write whatever you want. And, if you choose to write code that generates unhandled rejections, you could overwrite the default behavior by adding an "unhandledRejection" event handler. Similarly, if you want to write code that throws uncaught exceptions, you can add an "uncaughtException" event handler. I fail to see the difference between the two except that exceptions are synchronous; whereas, Promise rejections are asynchronous.

Thoughts?

EDIT: I'll agree the code written by @LinusU is something that I might write myself before realizing that I made a mistake... that I've (perhaps) left a few promise rejections unhandled for a short period of time. Furthermore, I'll argue that (more than likely), this bug would linger for quite a while before the process unexpectedly encounters an unhandled rejection (the default behavior for which (as I argue) is process termination).

This comes down to choosing the lesser of two evils:

• Option A: I don't care about unhandled rejections, but I do care about keeping my process alive. I will let them occur silently.
• Option B: I definitely care about unhandled rejections, especially if they aren't handled within a short period of time. I'd rather catch bugs than keep my process afloat.

Option B seems to be more inline with the behavior for uncaught exceptions. Why should unhandled Promise rejections be treated differently? Because it's _non obvious_ to write code that ensures promise rejections never go unhandled? How important is that really? It only matters when a rejection is likely to happen, right? The same argument could be said about exceptions. Suppose you're lazy and don't feel like writing try {} catch(e) {}. Your program could go on and on and work perfectly fine... until an exception actually occurs.

By the way, I think that this is an important debate, and I'm still on the fence about all of it. Thoughts?

[bminer]

A couple more things to add here....

Unhandled promise rejections are not a _good_ thing. As I understand it, the whole point of the "unhandledRejection" event is to have an opportunity to handle the promise rejection... or (more likely) report a bug. Therefore, IMHO, writing code that leaves Promise rejections unhandled is not good practice. If you do this, you might end up reporting bugs that aren't really bugs... because the rejections end up getting handled "later". How much later is undefined, so the "unhandledRejection" event handler doesn't really know what to do.

Another point is that default behaviors can be overwritten (at your own risk).

By default, library writers who write code that leaves promise rejections unhandled incur a penalty for doing so; their code is actually buggy, and it can cause a process to crash. I'd argue that the original example provided by @LinusU has a bug -- it allows a promise rejection to remain unhandled for an indeterminate amount of time; therefore, an "unhandledRejection" event handler wouldn't really know what to do. Report this as a bug? Wait 30 seconds to see if the promise rejection eventually gets handled? Do we get paranoid and kill the process? Who knows? Writing an "unhandledRejection" event handler in that universe is... kinda weird and awkward.

If we just silently ignore the promise rejection by default, I fear that Node libraries will contain code that leaves promise rejections unhandled. It's (admittedly) easier to do, so that's what would probably happen. If you were running a production app and wanted to report "unhandledRejections" as bugs, you could end up catching bugs from all over the place (from 3rd party libraries, maybe even from Node internal libraries), making the "unhandledRejection" event handler pretty unhelpful.

We all write buggy code; no one is perfect. How are we going to catch these bugs if we all started using Promises? Is an "unhandledRejection" event handler the answer?

[Fishrock123]

Fwiw it's still on my plate to try to make unhandledRejections warn to console.error on GC. (The only want to truly know that it is unhandled, what a mess.) Will take some work but I hope to get around to it soon.

[benjamingr]

GC will give you a lot of false negatives. Otherwise we'd fire the events based on GC too.

[LinusU]

@benjamingr How will that give you false negatives?

[Fishrock123]

GC will give you a lot of false negatives. Otherwise we'd fire the events based on GC too.

I think you are mistaken. It's not possible for a GC-based trigger to give a false positive.

If that happens an error has leaked in your system and you have problems.

We can't fire an event though, or even if we did, it would be useless, since then the promise has been GC'd and we don't have a reasonable reference to it.

(Ok technically v8 can probably resurrect it but that is almost certainly a horribly twisted idea.)

[benjamingr]

I don't understand.

function foo(){
    var p = Promise.reject(Error());
    function f(){ return p; }
    function g() {}
    return g;
}
longLiving = foo();

IIRC in v8 g would leak p because of how closures are implemented in v8.

Definitely it can't be handled. Not to mention getting a day-old console.error if a promise makes it to the old heap.

[benjamingr]

To clarify- you would not get false positives but you'd get a lot of false negatives.

[MicahZoltu]

I think you are mistaken. It's not possible for a GC-based trigger to give a false positive.

If that happens an error has leaked in your system and you have problems.

This statement operates on the assertion that unhandled promise rejections are a programming error. The canonical counter-examples to this are pre-loading and fire-and-forget. In the first case, if the pre-loaded resource is never accessed then no one cared that it failed so no one hooked up a rejection handler. In the second case, the user is attempting a best-effort operation and failure is intentionally unhandled because the author doesn't care.

I believe further up in this thread there are details on why these are acceptable programming paradigms and why "just attach a handler to everything" isn't a viable response, but I don't mind going over them again if it is desirable.

[Fishrock123]

To clarify- you would not get false positives but you'd get a lot of false negatives.

Sorry, I read that wrong.

How though? GC is only going to happen once nothing has a reference the the promise and there is nothing that can influence it. If you have an error in there, you have an error.

heck, we should probably actually throw that error then, but apparently promises are a weird land where the rest of JS doesn't apply, so we agreed on ages ago that warning was the best idea.

[bminer]

@Zoltu - I would assert that unhandled promise rejections are programming errors... or at the very least poor practice. For pre-loading, just attach a do-nothing rejection handler. How is that different from writing an empty catch block? The author could choose to not write an empty catch block and risk an uncaught exception, but it's probably not a great idea. Same for fire and forget. Why is the "just attach a handler for everything" not acceptable? Again, how is this different from try/catch in synchronous code?

[LinusU]

Preloading should not be a problem? Unless you preload stuff and then throw it away without ever using it...

[MicahZoltu]

class Foo {
    constructor() {
        this.authenticationPromise = authenticateWithCachedRefreshToken();
    }

    // user tries to do something that requires authentication
    onUserInteraction() {
        this.authenticationPromise.then(userProfile => {
            // TODO: use user profile to give results to the user
        }).catch(error => {
            // TODO: show the login dialog to the user with red error for failure
        });
    }

    // called when user submits login dialog
    onLogin(username, password, whatTheyWereTryingToDoBefore) {
        this.authenticationPromise = authenticateWithCredentials(username, password);
        whatTheyWereTryingToDoBefore();
    }
}

In this example, we opportunistically try to authenticate the user using a refresh token from local storage. In some cases, we'll be done (success or failure) before the user interaction occurs. In other cases, we won't be so we have to execute the user interaction after resolve/reject. Once resolve/reject occurs, we can then handle the resolution or rejection. In this example, in the case of rejection we prompt for credentials and then try auth again with the credentials, reusing the authenticationPromise so any user interactions that occur while waiting for credentials will be delayed until authentication completes. This allows us to handle the case where the user submits login credentials and then clicks something else before authentication completes.

The problem with unhandled rejection logging is that this code will print an error if the user never does an interaction that requires authentication! We opportunistically tried to log the user in in order to give them a good user experience, but that may fail. If we attach a catch handler to the result of authenticationPromise then the rejection is swallowed and we can't later .catch().

The workaround people have proposed in the past is this:

constructor() {
    this.authenticationPromise = tryAuth();
    this.authenticationPromise.catch();
}

The problem with this is that it results in a _lot_ of code just to work around the fact that the runtime is doing logging errors when it _thinks_ a mistake was made (not when a mistake was actually made).

If you are programming with a lot of promises, this sort of pattern becomes pretty common and having to turn every line of code into 2 lines sucks, especially when they occur as expressions that now need to be wrapped in a code block and have a return statement added. One general way around this is to wrap your core libraries with an immediate catch on all promises just to silence the runtime's logging, but that is also a pretty crappy solution.

As I have said previously in this thread, this whole problem goes away with async/await. By adding unhandled rejection tracking in now, all we are doing is crippling promises (making them harder to use in some scenarios) to work around a problem that only exists while we wait for async/await.

[benjamingr]

@Zoltu I think you should probably consider reading what platforms _with_ async/await do and why (C# and Python for example are _extremely_ opinionated. C# will even crash the request on a _pending_ async operation (even if it was not rejected) by default). Also - async/await doesn't actually solve anything here.

If we ignore the question of whether performing i/o in the constructor is a good/bad idea, I'm perfectly content with people having to be explicit about suppressing un-handled application and programmer errors.

[MicahZoltu]

C# will turn a rejection into an exception on await, which I believe to be the correct behavior. If you interact with the task directly (no await) then an exception is never thrown, which I also believe the be correct behavior.

When I researched this previously, only dart (I believe) had unhandled rejection behavior. Every language with promises/futures/tasks did nothing on unhandled rejection/resolution.

The reason async/await solves this problem is because awaiting a promise will turn rejections into exceptions for you, so you can't forget to catch. If you use await to access your variables you can just wrap your code in a try/catch block and pretend it is sync (mostly). Assuming you are using await to access your promises, you don't need to worry about unhandled rejections because they are turned into exceptions at the right time (on attempted access).

I'm perfectly content with people having to be explicit about suppressing un-handled application and programmer errors.

The example I gave was not a programmer error. The application is behaving exactly as expected and has a better user experience than a more traditional model of kicking off the request in the same place you know how to handle the rejection.

[bminer]

Boy, this has certainly become a religious debate, and I feel like I'm late to the party. :)

TL;DR -- Unhandled promise rejections are bad, MKAY!

@Zoltu - async/await does nothing to solve the problem. await is an _explicit_ way to add a rejection handler, and this line should be reached _before_ the promise actually rejects (edit: unless it is rejected synchronously). If you forget to write await or your program unexpectedly branches such that await is not reached, you can have an unhandled promise rejection. Therefore, to promote good programming practices, unhandled promises should really be avoided -- you should add handlers for the rejections before the promise has an opportunity to be rejected. Similarly, you should probably add the handler for the promise success before the promise has an opportunity to succeed... although that's far less important for this discussion.

The example I gave was not a programmer error. The application is behaving exactly as expected and has a better user experience than a more traditional model of kicking off the request in the same place you know how to handle the rejection.

I'll start out by saying that there are many ways to solve a problem. Authentication is an interesting one. One question I propose is this: Do you reject a Promise if login credentials are invalid, or do you "successfully" authenticate as a null user? IMHO, promise rejections should be pretty rare; they are the asynchronous counterpart to a synchronous exception. It's hard to argue otherwise because an await turns a promise rejection into an exception! Therefore, IMHO an authentication promise should only reject when there is a problem talking to a database, connection is dropped, etc; it shouldn't reject just because the user's credentials are wrong. In synchronous land, this is the argument between returning null vs. throwing an exception. My point here is that there should be some effort to make promise rejections just as rare as exceptions (for performance reasons and for good practice).

If promise rejections were rare in your authentication example, you could probably silently ignore them. Nothing wrong with .catch(nop) in certain circumstance. Similarly, there's nothing wrong with catch(e) {} in certain circumstances.

Now, I'll back up a bit. We can throw examples out all day long. At the end of the day, an unhandled promise rejection is _not_ a good thing, whether using async/await or not. As I've mentioned before, this is akin to not adding a catch(e) {} block. As an author of code, you're certainly allowed to do this, but it's probably not a good idea if exceptions are possible (i.e. reading the contents of a file). Async code makes this slightly more complicated because an unhandled rejection _might_ be handled... sometime later. Since "sometime later" is undefined, it's best just to assume that unhandled rejections that are left unhandled after the next event loop tick result in uncaught exceptions.

I think that generally speaking there is a _better_ way to write code so that unhandled rejections are completely avoided; that is, to add rejection handlers right up front.

[MicahZoltu]

@bminer Yeah, you are entering into a rather hot topic. :)

I think fundamentally there are currently two popular mental models of promise. One of them is that rejections are the same as exceptions (as you have described) and the other is more akin to a union type that is switched on.

The following two sequential code blocks describe the two mental models (no async work in either):

try {
    let result = takeAction();
    processSuccess(result);
} catch (error) {
    processFailure(error);
}

let result = takeAction();
if (result.isSuccess) {
    processSuccess(result.success);
} else {
    processFailure(result.failure);
}

Once async enters the picture, without promises that first bit of code becomes difficult to write as we have seen in all of browser JS history. People _want_ to keep the simpler sequential mental model yet work in a non-blocking way so they use promises like this:

takeAction().then(result => {
    processSuccess(result);
}).catch(error => {
    processError(error);
});

This is very reasonable and I'll fully admit to using promises in this way. However, what I _really_ want isn't the above but instead I want the following:

try {
    let result = await takeAction();
    processSuccess(result);
} catch (error) {
    processFailure(error);
}

Taking a step to the side for a moment, there is another usage pattern that is looking for a solution and that is the complex application that is leveraging promises for more than simplifying async code into a sequential mental model. It is fully leveraging promises across the board to deal with the fact that anything can happen at any time and multiple things may want to share the results of some async task. This scenario is harder to draw up but the general idea is:

export class FooService {
    barPromise: Promise<Bar>

    constructor() {
        this.barPromise = getBar();
    }

    actionA() {
        this.barPromise.then(bar => {
            // work with bar in a way that is meaningful to actionA
        }).catch(error => {
            // handle errors getting bar in a way that is meaningful to actionA
        });
    }

    actionB() {
        this.barPromise.then(bar => {
            // work with bar in a way that is meaningful to actionB
        }).catch(error => {
            // handle errors getting bar in a way that is meaningful to actionB
        });
    }
}

In this example, there are a bunch of different actions (two shown) that can't be completed until bar is available. They want to share a bar, but there is no guarantee as to which action will occur first or whether or not getting bar will succeed. It is also possible that neither action will ever occur. In this scenario, as application complexity goes up it becomes more and more difficult to guarantee that every promise in the system has a catch attached to it. This is especially true when you start crossing library boundaries because you can't make any assumptions about what a library does internally so _every_ promise that passes through a library needs to have a catch wrapping it on the other side.

For completeness, this is what the above looks like with async/await:

export class FooService {
    barPromise: Promise<Bar>

    constructor() {
        this.barPromise = getBar();
    }

    actionA() {
        try {
            let bar = await barPromise;
            // work with bar in a way that is meaningful to actionA
        } catch (error) {
            // handle errors getting bar in a way that is meaningful to actionA
        }
    }

    actionB() {
        try {
            let bar = await barPromise;
            // work with bar in a way that is meaningful to actionB
        } catch (error) {
            // handle errors getting bar in a way that is meaningful to actionB
        }
    }
}

Lets fast forward to the future when the async/await examples are possible. You now have two use cases to consider. One is the use case where you have a developer that wants to use a sequential mental model while leveraging non-blocking IO under the hood. In this scenario, they simply use async/await and likely never have to work directly with a promise. If they forget a catch block, they'll get an exception in the console. As long as the user isn't doing anything advanced with promises, they are fully covered and can live in a sequential world despite doing async under the hood.

In the other scenario however, the user is leveraging promises more fully. They are using them as one would use a Future in Java or a Task in C# to share delayed results across multiple usage scenarios throughout a system, allowing the developer to avoid duplicating work while still being able to interact with the promise in a sequential way once they are ready to handle it.

How do unhanded rejections play into all of this? In the sequential mental model, they are unnecessary once async/await lands because you can no longer "forget the catch" if you are using async/await. If you aren't using async/await, then you are _most likely_ using promises in a more advanced way in which case the unhanded rejections are likely to be intentional.

It is still possible to forget a catch when using promises directly, but I contend that the developer using promises outside of async/await is likely a more advanced developer doing advanced things and the burden on them to not screw up is higher and this is an acceptable cost to allowing for these advanced usages of promises.

TL;DR:

Promises are a building block for a couple different things, one of them is async/await. By themselves, promises are not a complete solution and we should not limit their capability because the next layer up (async/await) is not finished yet. By thinking of and designing promises as a success/failure union you allow them to be used for more than just the async/await use case and they can still solve the problem of wanting to do async work with a sequential mental model with an exception style safety net once async/await lands.

[mikeal]

At this point I'm going to suggest that this move to a PR for one of the many proposals. This issue have gotten too broad to be productive and it's probably time for whoever feels most passionate about a particular proposal to send a PR and people can debate its merits in a more solution oriented context.

+1 for closing/locking with a note that PRs are welcome.

[MicahZoltu]

@mikeal How does one submit a PR for "Do Nothing"? ;)

Seriously though, the PR approach _strongly_ favors the camp of change over the camp of no-change. Also, I believe there are already PRs out? Though maybe I'm thinking of io.js...

[bminer]

@mikeal - I'm sorry for spamming everyone, but I feel like this debate is important. A lot of developers still write callback-style async code, but since promises are gaining in popularity, someone needs to make a good, well-informed decision here (even if they don't agree with me) for the sake of future Node developers. :) Callbacks sounded like a good idea until we one day "discovered" callback hell... we're all trying to find a better way to develop software.

Anyway, I'll agree that the Github issue tracker probably isn't really the appropriate place for this sort of debate (nor is a PR probably), so I don't mind moving the discussion/debate someplace else. Are there any forums for this sort of thing?

@Zoltu - Thanks for your response. As far as I can tell, your argument is that unhandled rejections are likely either irrelevant (in the case of using async/await, since you'll probably handle them later) or intentional (because you're using Promises directly in a more advanced way). Then why have an "unhandledRejection" handler in the first place?

What about simple syntax errors in Promises?

var promise = new Promise(function(resolve) {
    kjjdjf();  // function that doesn't exist because I failed at typing
});
// This results in an unhandled promise rejection, not an exception

How would bugs like these end up getting caught? Are you saying that I'm using Promises without async/await, so we should still discard the error?

[MicahZoltu]

@bminer There is a more thorough discussion on this topic for ES6 in general over here: https://github.com/promises-aplus/unhandled-rejections-spec/issues

In your example, if you are using async/await you would get an exception as soon as you tried to access the result of the promise:

var promise = new Promise(function(resolve) { kjjdjf(); });
// some other stuff
await promise; // throws

If you aren't using async/await you end up with something like this:

var promise = new Promise(function(resolve) { kjjdjf(); });
// some other stuff
promise.then(() => {
    // never called
}); // no throw!

This second example is the problem that everyone is trying to solve with unhanded rejection tracking and I acknowledge that it is a reasonable problem to try to solve, I just don't think it is worth putting in a poor solution to it today when there is a much better solution to it (await) coming tomorrow.

To be clear, I am arguing against any unhandled rejection handlers, though particularly default unhandled rejection handlers that have visible side effects like crashing the app or logging to the console. Chrome currently automatically logs all unhandled promise rejections to the console and as a result my app's console is flooded with rejection handlers because I do late-binding of the handlers (like the auth example previously).

[ivan-kleshnin]

Throwing an error is the sanest default option fo my taste.

I write a lot of utility scripts in JS and I'm deadly bored by

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

mantra or equivalent import in _every_ independent JS file. Some scripts can be written sync-only but as soon as you get http or sql where there is no sync equivalent, it's coming.

And, to be honest, provided arguments for Logging or Doing nothing do sound like a joke to me.
The only valid counter-argument is that damned backward compatibility.

[MicahZoltu]

@ivan-kleshnin when would the error be thrown and what would the stack trace look like?

[ivan-kleshnin]

@Zoltu not sure I'm getting you correct. Can you elaborate?
I'm ok with one more item in stack trace. I'm also ok with the possibility to cloak unhandled errors if its intentional i.e. explicit. Hiding errors by default smells like PHP and breaks my trust in platform.
Especially, when in "errors" we include "syntax errors" like @bminer already stated.

[benjamingr]

@Zoltu that's not what C# does at all. C# has a concept of an execution context - for example ASP.NET will _fail the request_ even if there are _no unhandled rejections_ just because there are async in flight actions created in the request context and not resolved by the time you're sending the response.

Aside from that:

I'm 100% with @mikeal here, I have another suggestion: we wait for the TC to finish the official promise hooks, wait to see what browsers implement and _in a year_ we go with _whatever behavior browsers have settled on by then_ since that will be _de facto standard_.

[bminer]

wait to see what browsers implement and in a year we go with whatever behavior browsers have settled on by then since that will be de facto standard.

@benjamingr - I don't mind the idea, but Node can set standards, too. :) Plus, browsers don't "crash" on uncaught exceptions; they have some goofy onerror handler thing and dump errors to the console. Node is just a different beast in this regard. Thoughts?

All in favor of moving this discussion to unhandled-rejections-spec issue page, say "Aye."

[benjamingr]

@bminar that site and spec are dead, maybe @domenic would like to revive them but honestly I think he's busy doing a million other things and getting browsers to add the hooks first.

Also - node _can_ set standards, but I think if users are used to the browsers logging to the console then similar behavior by default in node might be a good idea.

@paulirish did you guys reach anything meaningful regarding the instrumentation when you investigated the promises tab? Is the process leading to the choice of "log unhandled rejections to the console" documented somewhere?

Similarly - who implemented this in other browsers?

[winterland1989]

Just come across this issue, it reminds me a month ago i present a new continuation based solution other than promise in the ES mail list, and everyone is trying to convince me promise is the right way to do it, after some search i also found this compositional-functions proposal.

Now i suggest everyone give a closer look at this proposal and my implementation, which IMO solves the most problem of promise, since the async-await proposal is going make its way into ES7, and the gate of choosing async primitives are closing, please pay some attention and thanks!

[LinusU]

@winterland1989 Your change from cb(err, data) to cb(dataOrErr) makes it instantly incompatible with every other node module out-of-the-box. I even like reject, resolve better than that. Why did chose to do only one argument and then introduce instanceof Error checking?

[winterland1989]

@LinusU Thanks for reading my implementation!

The reason why cb(dataOrErr) interface was chosen is simplifying error handling, and this design choice make the implementation of retry/race/parallel... much cleaner. And i admit it's somehow an arbitrary design choice.

Of course this is just an implementation detail of compositional-functions proposal, and we can open a discuss on the interface of final implementation if this really annoy a lot of people.

The bottom line is, since javascript is somehow a functional language, we don't really have to lock ourself with a state machine based solution(promise), which has obvious flaws, such as error handling and cancellation. Borrow some functional idea such as continuations and call-cc from other language doesn't hurt.

[benjamingr]

I'm going to close this as we've been unable to progress with this discussion or with any proposal.

PRs and further discussion welcome.

[noinkling]

@Zoltu

In this scenario, they simply use async/await and likely never have to work directly with a promise. If they forget a catch block, they'll get an exception in the console.

Correct me if I'm wrong or if I'm speaking out of place, but that's incorrect isn't it? Use of await requires an enclosing async function (I noticed that this was missing from your examples), so if you forget to use try/catch it just becomes the rejection value for the promise that _that_ function returns instead, and you're back to where you started - you have to catch it _somewhere_ or else it will be swallowed, therefore async/await has the exact same issue.

I only bring this up because, as a newcomer to Node, I ran into this myself recently while using async/await through Babel. If I'm any indication of what's to come, there will be people staring at their log output for half an hour wondering why their incoming server request is failing silently and timing out in the browser. In fact I did actually have a try/catch block in my function, but the error was occurring in a (decidedly synchronous) JSON.stringify call that I had ignorantly left outside it (lesson learned). If the unhandled rejection had been logged by default (much like it is in Chrome) I would have presumably seen the word "unhandled" along with the error and instantly known what I did wrong. As it stands I will be adding process.on('unhandledRejection', err => { console.error('Unhandled promise rejection ', err.stack); }); or similar to every Node application I build because of how informative it will be if I or anyone else using my code messes up again.

[MicahZoltu]

@noinkling Hmm, in C# you aren't allowed to use await inside of a function that isn't marked as async. I didn't realize that ES allowed you to have a non-async function with await used inside of it. I never really thought about it before, but I wonder if this sort of problem is why C# forces you to mark your methods with await as async?

[noinkling]

@Zoltu You misunderstand me, I'm saying that ES _does_ currently require an enclosing async function in order to use await (though as I understand it, top-level await is being considered). You left it out in the examples you gave. Therefore, since any await requires an enclosing async function (which returns a promise), with the current Node behavior errors will never be logged by default, they will be swallowed just like any other promise unless explicitly handled, even if they are 'transformed' temporarily to exceptions within the function.

In other words, async/await doesn't solve anything with respect to the logging problem, which makes sense, since it's based on promises under-the-hood.

[MicahZoltu]

Ah, you are right, I misunderstood you and I think you are correct, that async/await only helps if you are using it everywhere. If you are using await to interact with promises exclusively then you won't have any trouble and there shouldn't be any need for unhandled rejection handlers. If you are sometimes interacting with the promise directly (e.g., .then(...)) then you have to be more careful.

In my experience with C#, very few people interact with Tasks (promise equivalent) directly. They all interact with them via the await keyword. The only people who interact with Tasks directly are people who need more advanced control, which is the same set of people who would benefit from no unhandled rejection handler.

[noinkling]

If you are using await to interact with promises exclusively then you won't have any trouble and there shouldn't be any need for unhandled rejection handlers. If you are sometimes interacting with the promise directly (e.g., .then(...)) then you have to be more careful.

There's no difference though (other than syntax) - either you use .catch with promises or try/catch with await, otherwise the error is silent. You have to handle it explicitly either way or nothing will be logged. To reiterate: async/await changes nothing in terms of logging behavior compared to promises, and therefore doesn't solve the issue of silent errors. In C# it might be different, I don't know.

[MicahZoltu]

Perhaps I am misunderstanding something but as I understand it if you _ALWAYS_ interact with promises via await then the only way you'll miss an exception is if you never looked at the result of a promise.

let fooPromise = getPromise(); // no throw here, we haven't tried to interact with the result yet
let barPromise = getPromise(); // will never throw, but since you never awaited the result it seems you don't really care
let foo = await fooPromise; // exception thrown here
actOnFoo(foo); // we'll either get here or we'll see an exception bubble up the stack

let fooPromise = getPromise();
let barPromise = getPromise();
fooPromise.then(result => {
    let foo = result;
    actOnFoo(foo); // we may never get here *AND* never see an exception, which is one reason why `await` is better
});

This practice depends on using async/await all the way up the call stack. Perhaps the issue you are referring to is the fact that at the very top (as you mentioned) you have to call your top level function. I agree, that Node should allow using await from top-level functions. In the meantime, I would recommend something like this:

async main() {
    // TODO: write application in here, use `await` everywhere interacting with promises
}

main().catch(error => console.log(error));

That being said, we are now getting into style questions and this style depends on not executing code in the global space (which seems to be very common in ES). C# has the same sort of problem, and you can see the same sort of answer here: http://stackoverflow.com/a/24601591/879046. Howover, since C# only has a single entry point which is a function, it isn't possible to accidentally execute code outside of the wrapped execution context so it is much harder to screw up than in Node.

[noinkling]

Let me make a fairer comparison:

async function doAsyncStuff() {
  await promiseThatRejects();
}
doAsyncStuff().catch(error => console.log(error));

vs

function doAsyncStuff() {
  return promiseThatRejects();
}
doAsyncStuff().catch(error => console.log(error));

Anywhere that await can be used to propagate an error, returning the promise can be used to the same effect (which is typically how you work with promises in order to maintain a flat structure). Of course, the wrapper function in the 2nd example is unnecessary in this case, but it's needed to make a fair comparison and in real code would depend on context (due to interacting with other APIs or whatever). This might be more realistic:

async function doAsyncStuff() {
  try {
    await getPromise();
    // ...
  } catch (error) {
    console.log(error);
  }
}
doAsyncStuff();

vs

getPromise()
  .then(...)
  .catch(error => console.log(error));

But that's irrelevant to the issue at hand.

One of the main reasons for promises and async/await existing in the first place is to avoid a deeply-nested call stack, so error propagation up a stack shouldn't be much of a concern in the first place... but if for some reason it _is_, then async/await offers nothing that standard promises can't provide in this regard, and the fact remains that neither paradigm will do any logging without the error being explicitly caught and logged somewhere (yes, top-level counts).

_If_ and _when_ top-level await makes it into the language, then I think you might possibly have a point, depending on how errors at that level are handled. That's a big "if" and a big "when" though.

If you still disagree, feel free to PM me, I didn't mean to hijack the thread with this discussion.

[vkarpov15]

I'm not gonna mince words, the "PromiseRejectionHandledWarning" default is crap DX. Much of the point of promises is that you can handle promise rejections asynchronously, now a bunch of perfectly valid promise patterns print spammy warnings unless you tweak the default.

before(function() {
  sinon.stub(obj.asyncFunction).returns(Promise.reject(err));
});

it('my test', function (done) {
  obj.asyncFunction.catch(error => {
    assert.ok(error);
    done();
  });
});

let lastPromise;

stream.on('data', () => {
  if (lastPromise) {
    lastPromise = lastPromise.then(() => doSomethingAsync(), () => doSomethingAsync());
  } else {
    lastPromise = doSomethingAsync();
  }
});

Etc. This warning is tangentially useful for debugging, but you need to remember to shut it off in prod and if you're doing anything even remotely non-trivial with promises.

[Fishrock123]

but you need to remember to shut it off in prod and if you're doing anything even remotely non-trivial with promises.

This would be correct, but being the default makes it usable by people who may not have as good of a hand on their potential problems.

[vkarpov15]

Not necessarily, just makes perfectly valid patterns look scarier than they are and makes npm modules that do perfectly reasonable things print warnings upstream, making it so end users have to worry about module behavior. It isn't node's job to fix promise hell, that's what async/await or co/yield is for. Then, when people start blogging about async/await hell, you can come up with another half-baked "kid wheels mode" hack like domains for callbacks or logging warnings to the console for promises :)

[bminer]

I still think that the default (yet customizable) behavior for unhandled rejections is to crash the app, just like an uncaught exception.

If you want to handle a Promise rejection after it occurs, you can bind your own "unhandledRejection" handler to do just that. But, we cannot just ignore unhandled Promise rejections by default. Your custom "unhandledRejection" handler could do something like start a 30 second timer to see if the Promise is eventually handled. But, eventually, all Promise rejections should be handled... if not, it's probably a bug, perhaps even a bug that should intentionally crash the app.

Unhandled rejections are potentially just as dangerous as uncaught exceptions IMHO. The app could be in an undefined state. Even SyntaxErrors can result in unhandled rejections, for example.

[vkarpov15]

So should every node module have its own special snowflake handler for on('unhandledRejection')? What if some module wants the behavior to be "crash the app" and another module wants it to be "ignore it"? Global behavior is global, no way for child libs to change it without risk of conflict. The point of this sort of feature is for the end user app to decide how it wants to handle unhandled rejections, not for node to decide what promise patterns should node modules be allowed to use, which is what it does now.

[bminer]

@vkarpov15 - To answer your question: Yes. IMHO, late binding a rejection handler is poor practice -- libs should avoid this pattern. If they really want to do it, they can have their own "unhandledRejection" handler.

Maybe Promises should have a flag to indicate that late binding is desired at the time of construction? Also, separate discussion but maybe Promises should have a .cancel() method to abort the async operation?

[vkarpov15]

It's far from bad practice, it's necessary practice for more advanced use cases, which is what modules should be providing wrappers around anyway. Correct me if I'm wrong, but the 'unhandledRejection' handler is global to the process as a whole, which means a module that wants to crash on unhandled rejections will screw over every other module. TBH I understand your point, if you'd asked me in early 2015 I would've made a similar argument, "bad practice don't do it". But getting more comfortable with promises and spending time in observables land has made me realize that restricting promises to "let me just chain a bunch of HTTP / database calls together" is really a narrow view of what promises can do, which is what this default limits you to.

[benjamingr]

This discussion has been had several times and the consensus is to warn on it now and throw on GC when we have the hooks to do so.

It is easy to attach an empty catch handler which will suppress the warning.

There are at least 5 threads like this one - I recommend you look at the NodeJS/promises one if you want more up to date arguments.

[bminer]

Referencing a relevant PR here: #8217
Process warnings will occur starting in Node 7

[rsp]

I wrote the caught module to simplify working with use cases described by @MicahZoltu and @vkarpov15 without globally changing the behavior of everything by listening to the events.

It was originally written as an example for this answer on Stack Overflow: Should I refrain from handling Promise rejection asynchronously? but maybe it could be useful to someone who reads this discussion.