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 notifications@github.com 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.