Default Unhandled Rejection Detection Behavior #830
Comments
Actually, logging to stdout (or stderr, I guess) can cause crash: nodejs/node-v0.x-archive#6612 |
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.
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
Does thrown error will not be caught by
As variant to give developers choose how to handle unhandled promise rejections |
|
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 |
|
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 |
|
The problem with doing nothing is that the feature might as well not exist as it will be undiscoverable to most developers.
This is exactly what the default uncaught exception handler does minus exiting the process.... |
|
I would say emitting an event eg At least, don't choose to do nothing. Errors which are silenced is a nightmare to debug. |
I am for no default as emitting event as @petkaantonov already implemented. It might be simply changed to process.on('unhandledRejection', function (err) {
throw err;
});
process.on('uncaughtException', function (err) {
log(err);
}); |
|
-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 — On Fri, Feb 13, 2015 at 6:48 AM, Dmitry [email protected] wrote:
|
|
+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. |
|
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 So promises swallow errors "by default" to the same extent callbacks do. This is about how we can be even better. |
|
... 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 The tradeoffs are very similar, so I hope your answer to that question can help you contextualize the question posed in the OP. |
Forgot to handle the I like the comparison by the way. |
It's not the same at all, callback errors are always/generally operational while promise rejections can be either programmer errors or operational errors. |
|
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. |
This is inaccurate. Chrome logs; it does not throw. |
|
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? |
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 In other cases you may accidentally write such code but it can be always refactored. |
|
@domenic can you link to the relevant parts of the streams spec that requires/needs attaching error handlers asynchronously? |
|
i believe these are serious programmer errors, so it should kill the process or be passed to |
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 |
|
@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 i didn't explicitly say that it should |
|
I think one of the very useful features of uncaught exceptions is the Having an This means I get the most important thing I want, which is a core dump with the actual stack of the exception. |
|
👍 for an |
|
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:
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? |
|
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 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. |
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 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. |
Correct me if I'm wrong or if I'm speaking out of place, but that's incorrect isn't it? Use of I only bring this up because, as a newcomer to Node, I ran into this myself recently while using |
|
@noinkling Hmm, in C# you aren't allowed to use |
|
@Zoltu You misunderstand me, I'm saying that ES does currently require an enclosing In other words, |
|
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 In my experience with C#, very few people interact with |
There's no difference though (other than syntax) - either you use |
|
Perhaps I am misunderstanding something but as I understand it if you ALWAYS interact with promises via 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 stacklet 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 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. |
|
Let me make a fairer comparison: vs Anywhere that vs But that's irrelevant to the issue at hand. One of the main reasons for promises and If and when top-level If you still disagree, feel free to PM me, I didn't mean to hijack the thread with this discussion. |
|
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. |
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. |
|
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 :) |
|
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. |
|
So should every node module have its own special snowflake handler for |
|
@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 |
|
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. |
|
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. |
|
Referencing a relevant PR here: #8217 |
|
I wrote the 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. |
To support a mixture of sync and async module loading, the ui5loader creates a Promise for any requested resource to track its loading state, even when the first requestor uses a synchronous API. As long as no asynchronous API asks for the same resource, this promise is never used. Modern browsers report a rejection of such a promise as 'Uncaught (in promise)' error and report it on the console. One solution would be to create the Promise only on demand, but then error tracking would be more complicated. As an alternative, this change adds a dummy catch handler to the promise. This has no impact on error reporting via APIs (errback - covered by tests). For some background, see the discussion about rejected promises ( nodejs/node#830 ) and the current answer of the HTML5 spec to the topic ( https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections ) Change-Id: I99e5c1384a4e5caf7ccdf21db1dfadbdc75884f3
To support a mixture of sync and async module loading, the ui5loader creates a Promise for any requested resource to track its loading state, even when the first requestor uses a synchronous API. As long as no asynchronous API asks for the same resource, this promise is never used. Modern browsers report a rejection of such a promise as 'Uncaught (in promise)' error and report it on the console. One solution would be to create the Promise only on demand, but then error tracking would be more complicated. As an alternative, this change adds a dummy catch handler to the promise. This has no impact on error reporting via APIs (errback - covered by tests). For some background, see the discussion about rejected promises ( nodejs/node#830 ) and the current answer of the HTML5 spec to the topic ( https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections ) Change-Id: I99e5c1384a4e5caf7ccdf21db1dfadbdc75884f3 CR-Id: 002075125900000687202018 (cherry picked from commit 47ac957) BCP: 1880209531
and others
With #758 (hopefully) soon landing to address #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
catchhandler (orthenhandler with a function second argument) attached.The different approaches
process.on("uncaughtException"to deal with.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
Throwing an error
Process Exit
Do nothing
What do people think the default behaviour should be?
The text was updated successfully, but these errors were encountered: