Question about distinction between operational/expected errors and programmer errors/bugs

[misterdjules]

In the error handling model used by a significant population of Node.js programmers, we expect the process to exit (or abort) when an error is thrown, but not when an error is passed as an argument to a callback function.

For instance, we expect this code to make the process exit early:

function foo(callback) {
  throw new Error('Boom');
  callback();
}

foo(function fooCalled(fooErr) {
...
});

But we don't expect this code to exit early:

function foo(callback) {
  callback(new Error('Boom'));
}

foo(function fooCalled(fooErr) {
...
});

In other words, consumers of the foo API are free to ignore errors passed to its callback, but they can't ignore any error thrown by foo or any of its dependencies.

I'm much less familiar with promises and how Node.js users use them, so please accept my apologies if this question is not relevant, but it seems there are two main ways to reject them:

1. Rejecting them by not throwing an error, by e.g. calling the reject callback passed to a promise's constructor, using Promise.reject, etc.

2. Throwing an error from within a promise's constructor or a promise's handler.

Is that correct?

If so, do promises users use those two different ways of rejecting a promise to convey different types/classes of errors? Do they consider promises rejected by not throwing an error to not necessarily represent bugs in their code but promises that throw to represent bugs in their code?

What about when a promise's constructor/handler calls an undefined function, or accesses to properties of null/undefined objects?

Would the proposal to exit a node process on any unhandled promise rejection by default have the potential to turn any rejection into a fatal event when this could not be the semantics of the code currently written by users of promises?

[benjamingr]

Is that correct?

Well, from polls I've conducted locally the vast majority of users use async/await.

(Polled 143 people who use JavaScript and attended a meetup unrelated to asynchronus stuff with the topics modules and async_hooks - 140 used promises, 137 used async functions)

Most users rarely use then or the promise constructor (except for API conversion) in new projects. Ideally I'd like the promise constructor to go away with a promisified core that would make it easier for users to write correct code.

Inside async functions - while people can await Promise.reject(...) they almost never do so - they instead opt to throw new Error(...).

If so, do promises users use those two different ways of rejecting a promise to convey different types/classes of errors?

They do not. Promise users typically use the type of an exception to make that call - or simply opt to crash the process on errors that might be operational. That is:

function foo(callback) {
  callback(new Error('Boom'));
}

With promises - foo would be an async function and the consumer would have to handle the error. I guess with the terminology you're using you can say that promises make not handling errors (even operational errors) a programmer error.

What about when a promise's constructor/handler calls an undefined function, or accesses to properties of null/undefined objects?

When an async function causes a ReferenceError then an error is thrown and the promise returned from that async function is rejected. Users in development get a warning in their console and users in production currently get the same warning unless they opt in to crashing (I personally do). This behavior changes in Node.js 11 (hopefully).

Most users of async functions I surveyed (lots of selection bias there though) think that an operational exception cannot be differentiated from a programmer one on the call site - or at least they believe that's not a decision the platform gets to make.

Would the proposal to exit a node process on any unhandled promise rejection by default have the potential to turn any rejection into a fatal event when this could not be the semantics of the code currently written by users of promises?

It could and I originally objected to doing that in a single semver-major that for that reason. Only after we did actual code analysis of how people use promises in practice I removed my objection to setting the default to crashing on GC in a single semver-major.

In practice because of the warning packages make sure their promises do not trigger the warning on by accident their side. People who write packages work hard to not cause the warnings and in practice I haven't been able to find a single package in the top 10000 packages that triggers the warning by mistake.

I also surveyed the code bases of 3 large(ish) companies using Node locally - two using native promises and one using Bluebird (with permission of course) and tried to trigger this behavior and couldn't.

While that is not a community-wide survey the people I polled seemed to be very in favour of crashing. Additional data to confirm or contradict my findings is of course welcome :)

[benjamingr]

On a side note - it is very unfortunate that there is no obvious way to filter errors and code has to be littered with if (err instanceof SpecificError) in places that handle errors rather than have better language level syntax for it.

I definitely think that's something we should bring up with TC39.

When I asked for it ~5 years ago I was told that pattern matching is right around the corner - clearly that wasn't the case :)

[misterdjules]

@benjamingr Thank you very much for the detailed answer, very much appreciated.

Also, just to be clear: I'm not suggesting users don't want the change in behavior that is proposed in nodejs/node#20097. It'd be much easier from the perspective of a post-mortem debugging user to have that behavior. What I'm trying to do with those questions is make sure we wouldn't be breaking a large part of the ecosystem that doesn't operate under the same assumptions.

Well, from polls I've conducted locally the vast majority of users use async/await.

(Polled 143 people who use JavaScript and attended a meetup unrelated to asynchronus stuff with the topics modules and async_hooks - 140 used promises, 137 used async functions)

It seems your poll resulted in roughly the same number of users stating they use promises and async functions. By "the vast majority of users use async/await", did you mean that they also use async/await in addition of using promises?

Also, do you have some info about how you conducted that poll and who was surveyed?

In general, having more information about all the surveys and research mentioned in your response would be very helpful.

Inside async functions - while people can await Promise.reject(...) they almost never do so - they instead opt to throw new Error(...).

What about outside of async functions?

If so, do promises users use those two different ways of rejecting a promise to convey different types/classes of errors?

They do not. Promise users typically use the type of an exception to make that call

Do you have some examples of this and data round how common this practice is?

or simply opt to crash the process on errors that might be operational

Is crashing on a rejection common among users of promises? Also, I assume you meant "to crash the process on errors that might not be operational", is that correct?

I guess with the terminology you're using you can say that promises make not handling errors (even operational errors) a programmer error.

That's only valid for promises created/used within an async function, is that correct? What about promises used outside of async functions?

Most users of async functions I surveyed (lots of selection bias there though) think that an operational exception cannot be differentiated from a programmer one on the call site - or at least they believe that's not a decision the platform gets to make.

If users think whether an error is operational or not cannot be determined by the platform, doesn't that conflict with several modes proposed by https://github.com/nodejs/node/pull/20097/files?

When an async function causes a ReferenceError then an error is thrown and the promise returned from that async function is rejected. Users in development get a warning in their console and users in production currently get the same warning unless they opt in to crashing (I personally do). This behavior changes in Node.js 11 (hopefully).

When you say "this behavior changes in Node.js 11", do you mean with nodejs/node#20097?

[misterdjules]

I'd also be interested in reading @ljharb's opinion on this topic.

[ljharb]

140 seems like an insignificantly small number of people to poll, but I never took statistics ¯\_(ツ)_/¯ I'd suspect that the vast majority of node users are most familiar with nodebacks, and don't have much of an opinion on promises at all. Using .then is quite common in code.

Unhandled rejections are, by design and in the language spec, not intended to terminate processing of the program - they are not conceptually the same as uncaught exceptions. There are indeed hooks in the spec that make "exit on unhandled rejection" possible, but these were added to have browsers add (and later remove, when needed) unhandled rejection warnings - browsers don't terminate the page's javascript when one occurs.

I continue to believe that the desire for aborting on unhandled rejections is a legacy attitude grown out of the historical pains of debugging async nodebacks, and it's something I'd prefer node never do.

[jkrems]

@ljharb Since node has neither "resources" (deterministic destruction) nor a concept of lifetime ("process-per-request"), it's not just about legacy concerns. Interrupting a computation in node has the very real risk of leaking sockets/file descriptors/other kinds of resources. And there's no mechanism to remedy that. I don't believe dismissing it as "this is a legacy concern coming from callbacks" is helpful.

[ljharb]

@jkrems leaks can be a problem from all kinds of sources, not just from unhandled rejections - and proper cleanup is ensureable regardless of promise use, or uncaught exceptions, or unhandled rejections. If someone's code is leaky, it's because they wrote leaky code - not because node has made a choice about unhandled rejections.

[benjamingr]

@misterdjules

Hey, thanks for the response :) Going to try to answer this - let me know if anything is unclear and I'll try to explain myself better.

What I'm trying to do with those questions is make sure we wouldn't be breaking a large part of the ecosystem that doesn't operate under the same assumptions.

It likely won't - and exit-on-gc based on those numbers sounds like a solid path forward without breakage in Node.js 11.x.x

, did you mean that they also use async/await in addition of using promises?

Async/await uses promises internally - so those people are using promises, but aren't actually writing .then very often at all or using new Promise for anything but converting APIs.

They do not. Promise users typically use the type of an exception to make that call

I don't have any hard data on this. Just people I've been asking around, StackOverflow stuff I've answered and random "raise your hands if" polls.

That said, I feel pretty strongly about maintaining ECMASCript semantics. There is no way we can make typos behave differently from database query connection exceptions at the callee-site without violating the Promise specification - even if we could do it in some way. I believe there is no way the TSC would approve any such change.

That's only valid for promises created/used within an async function, is that correct? What about promises used outside of async functions?

That is also correct for promises created outside of async functions. Forgetting to explicitly handle any error - even an operational one will cause Node.js to emit a warning (and crash in Node.js 11 hopefully).

Also, do you have some info about how you conducted that poll and who was surveyed?

I actually did this a few times - but I quoted one result.

I did the following for surveying so far:

• I polled the JavaScript Israel group for answers.
• I polled people when presenting in conferences and meetups - namely in the Node.js Israel meetup group.

We've talked in the Node.js summit with the foundation about doing a foundation sponsored community survey and they have been open to it. If you want to collaborate on making it I'd love that - I hope to survey the community about results methodically and in a peer-reviewed way so we can come to uncontested conclusions of how people write Node.js code :)

If users think whether an error is operational or not cannot be determined by the platform, doesn't that conflict with several modes proposed by https://github.com/nodejs/node/pull/20097/files?

Why would it be? Promises make not handling errors (even operational errors) a programmer error

Exiting when a promise rejection is unhandled and crashing the process may be the only reasonable thing to do. If a database connection release method failed to release a connection from the pool that is an operational error - but if a programmer forgets to handle this case restarting the server is the only safe thing to do.

When you say "this behavior changes in Node.js 11", do you mean with nodejs/node#20097?

Yeah, I do :)

[benjamingr]

@ljharb

and don't have much of an opinion on promises at all.

The vast majority of Node.js users according to not just my own findings but big polls - According to RisingStack's poll surveying 1156 developers ~75% used promises. Note that this poll is before native async/await.

Unhandled rejections are, by design and in the language spec, not intended to terminate processing of the program

I would love to see anything in the spec supporting this claim. As far as I know the language specification is not concerned with what terminates a program at all. Just like process.exit(0) exits Node.js and throwing an uncaught exception does whereas browsers have no analogs.

but these were added to have browsers add (and later remove, when needed) unhandled rejection warnings - browsers don't terminate the page's javascript when one occurs.

No one is suggesting using those hooks for terminating the process. Those hooks that were co-specified with Node.js own unhandledRejection hooks are only meant for warnings.

The suggestion is to use garbage collection for terminating the process which has been supported by the people who wrote the promises spec and worked on the concept. This does not have the false-positive issue of the hook and the warnings since you can prove a promise cannot be handled any more at that point.

I continue to believe that the desire for aborting on unhandled rejections is a legacy attitude grown out of the historical pains of debugging async nodebacks, and it's something I'd prefer node never do.

Let's discuss this for a minute. What sort of evidence would you like to see that this is not an attitude grown out of legacy? I have been working on stuff related to promises for the last 5 years and have been writing promise-based code for that long. I believe it is absolutely necessary to do the 'abort on gc' stuff in the long run to improve developer lives.

What sort of data would help?

[benjamingr]

@ljharb

If someone's code is leaky, it's because they wrote leaky code - not because node has made a choice about unhandled rejections.

👋 Node.js core has leaky code. We got leaky code. The reason Node.js exits on uncaught exceptions is also because core does not clean up after itself correctly on uncaught exceptions (or unhandled rejections for that matter).

I'd be all for a move towards making Node.js robust and logging uncaught exceptions "good enough", however it is way too easy at the moment to leak stuff just by using Node.js own APIs.

I'd definitely be into not-crashing on unhandled rejections at that point.

[ljharb]

@benjamingr the spec certainly does not forbid exiting on an unhandled rejection. However, Promise.reject(); creates one, and I think it's a bit absurd to think that it would be an expected result for that statement to cause the program to exit, even on GC.

I'm glad that node has moved towards exiting only on GC, because I think that's much less hazardous - but I still don't agree that it's a good idea to be the default.

I think that it's fine if the creator of a Promise wants to opt in to any semantics they want - but I don't think it's fine that user code (third-party or first-party) will get those semantics unexpectedly just because they didn't want to add a .catch() nor did they want to care about a rejection, as both browser usage and the design of the language might lead them to expect.

[jkrems]

I'd be all for a move towards making Node.js robust and logging uncaught exceptions "good enough", however it is way too easy at the moment to leak stuff just by using Node.js own APIs.

I'd also like to point out that even if node core doesn't have leaky APIs, not all APIs are self-contained. Any API that allows resource creation and cleanup as separate steps (as is the case with anything that works like open) has this issue, generally speaking. Because node's APIs cannot verify that userland didn't throw "right before" they wanted to close a resource.

Yes, the problem is people writing code that has bugs. Yes, those bugs could be described as "leaky code". But "just don't write bugs" isn't a solution..?

[ljharb]

Certainly not! but making unhandled rejections - a normal, acceptable thing in the language - suddenly risk exiting the program is an extremely blunt instrument. It also makes node deviate farther from browsers, which doesn't seem like it matches the general trend inside node core.

[benjamingr]

the spec certainly does not forbid exiting on an unhandled rejection.

👍

However, Promise.reject(); creates one, and I think it's a bit absurd to think that it would be an expected result for that statement to cause the program to exit, even on GC.

Ideally, if no one is .catching that then I'd expect that to exit - preferably ASAP and with a good stack trace and debugging experience. Promise.reject() is like throw undefined; - it should behave the same way in Node.js.

In the real world the heuristic we use now has false positives because someone can handle the rejection much later. That's why 'exit on GC' is appealing because even if there are false negatives there is an upside.

suddenly risk exiting the program is an extremely blunt instrument.

We have been emitting a runtime warning for over a year now (since v7.7). The warning literally said (even on false negatives) that the Node.js process will terminate on it in the future. That warning warns against much more than what we'll actually abort on.

It also makes node deviate farther from browsers

Node.js will be as far as it was before - exiting on uncaught errors. I wish our code was good enough to not have to do this - but it's not.

[ljharb]

Promise.reject(); is more like try { throw undefined } catch {}.

What I've seen in practice is that people ignore the runtime warning, or add their own handler to silence it.

[benjamingr]

That's Promise.reject().catch(() => {}) - Promise.reject() explicitly does not handle the error.

What I've seen in practice is that people ignore the runtime warning, or add their own handler to silence it.

That's entirely different from what I've seen - we did GitHub search surveys in 2017 and 2015 about it. (There is a list here: https://gist.github.com/benjamingr/0237932cee84712951a2 and I'll dig up the 2017 stuff).

[benjamingr]

@ljharb and again, I am very open to collaborating on the foundation survey. @dshaw has offered to help us with this in the summit and I'm looking forward to doing it.

I am only acting on data I gathered myself on my free time for the presentation in the summit - I'm very interested in different experiences and viewpoints (which is why I really appreciate your participation and different perspective). If you have any data indicating something else from what the surveys showed I'd love to know so we can make sure we include the more controversial stuff in the poll.