process: remove unhandled rejection deprecation warning

[devsnek]

Removes the deprecation warning that node will start crashing.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2410/pipeline

[ljharb]

yay, how soon can we backport this

[addaleax]

Do you have a reference for that decision?

Also, I’m not sure, but I feel like we could at least partially go into the announced direction and set process.exitCode = 1 in case of an unhandled rejection. How do you feel about that?

[ljharb]

That makes no sense at all; the whole point is that an unhandled rejection is not a bug, it's a normal part of the language, and having one does not necessarily mean anything failed in your application.

[devsnek]

@addaleax there is a google docs of the notes from a meeting at jsconf eu in october where this decision was made. i have absolutely no idea where that google docs is, but i assume someone else here will have it. maybe @BridgeAR or @benjamingr?

as for the exit code stuff, the plan is to add opt-in modes for handling unhandled rejections, so that would be part of a separate pr.

[BridgeAR]

There was no intention of removing the deprecation message. In fact, one part was to keep everything as is for now besides adding opt-in possibilities to a) deactivate these warnings with a runtime flag and b) other behavior as e.g., throw an error.

[devsnek]

@BridgeAR sure, but since crashing isn't the default, the deprecation message should be removed. i'm not removing the warnings.

[BridgeAR]

@devsnek the decision about the default was postponed. Therefore I would not want to remove the deprecation message either. We just agreed upon what should be added and to review that change after a while again to decide on the best default behavior.

[devsnek]

@BridgeAR the behaviour suggested in this warning makes no sense as a default, and there's no realistic way it could be so. If a specific application wants to treat unhandled rejection as a failure then it can do that, but we can't start passing opinions on how we think people should use js...

[BridgeAR]

@devsnek removing the deprecation message has not yet been part of the discussion on the last collaborator summit as the focus has been elsewhere. This should be a discussion on it's own. I would be fine to switch the "will" to "could" but removing it at this time does not seem right to me. I am happy to discuss this as soon as we have a joint decision how to deal with rejections in the future but that's still open.

[devsnek]

how about we discuss it now? no time like the present.

[BridgeAR]

We do not yet know what exactly a future default will be and what it's going to look like. I personally like to have the deprecation in place until then since we had consensus to include the deprecation message in the first place and AFAIK that was not a trivial process either.

[ljharb]

If there's no consensus to do what the message says, at the very least, can we change the wording to be more accurate?

[benjamingr]

I'm -1 on making any changes to the current mechanism at the moment before consensus is reached.

I'm +1 on following the work of @misterdjules and the consensus from interactivejs to turn on the advertized behavior of crashing by default on unhandled rejection.

Alternatively I'm also +1 on unhandled rejection crashing the process on GC but it looks like that's not the way we're going.

[benjamingr]

@ljharb

That makes no sense at all; the whole point is that an unhandled rejection is not a bug, it's a normal part of the language, and having one does not necessarily mean anything failed in your application.

Note this is not the consensus in the project nor in the ecosystem. This is also not what the polls showed.

[devsnek]

node is not a platform for fixing things you don't like about the design of JavaScript. if you disagree with the design of promises take it up with tc39 or use another language... the base rule of node should be "it runs JavaScript programs" and this breaks that. I don't want to flip a flag to unbrick node because someone else thought their interpretation of a language feature was more valid than mine. this is absolutely preposterous.

[ljharb]

The consensus, as i understand it, is not to enable the behavior by default. Thus, the error message is incorrect - so at the least, it should be changed to be less dire, and ideally, removed entirely.

[benjamingr]

node is not a platform for fixing things you don't like about the design of JavaScript. if you disagree with the design of promises take it up with tc39 or use another language...

@devsnek This is a really aggressive tone that does not encourage collaboration, it is also stating an opinion (that unhandled rejection tracking is somehow at odds with the language) as if it is a fact.

I am really not sure what you hope to accomplish with this tone - but I am going to disengage from this debate at this point.

[devsnek]

@benjamingr I don't think tracking unhandled rejections is at odds with the language... I've put in a lot of work in the past to improve our APIs around tracking unhandled rejections. What I'm against is making valid programs throw by default.

[devsnek]

@nodejs/tsc given that the person blocking this has elected to remove themselves from the conversation, what is the path forward here?

[joyeecheung]

FWIW this is on the agenda of the next diagnostics summit in two weeks: nodejs/diagnostics#203 there probably won't be any decisions made but there may be some valuable inputs.

[mcollina]

I object. Where was this decided?

[mcollina]

This should not land without a tsc vote.

[jasnell]

Hmmm.. that's not an actual status we have. May need the TSC to discuss how we generally want to handle this kind of case so it's not just a one-off

[devsnek]

I copied the term from 0037 and 0038.

[jasnell]

Yes, that's fine, but we still don't have an actual process for revoking a deprecation. That's what we should settle on. So far it's been done on a case by case basis.

[devsnek]

@mcollina this hasn't been decided yet, but as a friendly reminder: node crashing on unhandled rejections would mean there are valid javascript programs that node couldn't run, which i would assume is unacceptable given that ".js" is in the name of this program.

[targos]

Is it clear to those who object that this only removes the DeprecationWarning that is emitted only once for the first unhandled rejection? UnhandledPromiseRejectionWarning is still emitted for every unhandled rejection.

[jasnell]

I'm -1 on this also. The TSC should decide this.

Also, process question for the @nodejs/tsc ... adding a deprecation is semver-major... should revoking it also be semver-major?

[jasnell]

@targos,

Is it clear to those who object that this only removes the DeprecationWarning ...

Yes, that's clear.

[addaleax]

should revoking it also be semver-major?

I’d say semver-minor. I don’t think it’s realistic to assume that people rely on deprecation warnings?

[mcollina]

@devsnek

this hasn't been decided yet, but as a friendly reminder: node crashing on unhandled rejections would mean there are valid javascript programs that node couldn't run, which i would assume is unacceptable given that ".js" is in the name of this program.

Can you please update the PR description? It currently states that it has been decided.

---

as a friendly reminder: node crashing on unhandled rejections would mean there are valid javascript programs that node couldn't run, which i would assume is unacceptable given that ".js" is in the name of this program.

As a friendly reminder, the following code leaks a file descriptor:

const http = require('http')
const server = http.createServer(handle)

server.listen(3000)

async function handle (req, res) {
  throw new Error('kaboom')
}

I think the Node.js runtime needs to provide some level of basic safety when people use it to write server applications. I've seen this so many times, it's extremely easy to forget to add a try-catch or .catch() handler.

We should really crash by default.

[mhdawson]

My thought is SemVer minor as well for removing a deprecation.

[ljharb]

@mcollina That you can construct code that benefits from crashing is not sufficient to decide anything; since Promise.reject() has no reason to crash and is valid JavaScript, it means that crashing isn’t the proper default.

Removing a deprecation imo is patch - it’s not adding anything new and it’s definitely not breaking anyone - but it’s fine to do as a minor too out of an abundance of caution.

[mhdawson]

I think the path forward is:

• Discussion at Diagnostic summit (March 7-8), add outcome of that to this issue
• If necessary tag with tsc-agenda to be discussed at following TSC meeting.

[devsnek]

@mcollina maybe we should update our server api to be aware of promises, not brick valid js programs.

@mhdawson sounds good to me

[mhdawson]

One other thought is it seems like we've had a few discussions (unrelated to this one) where it comes down to how cautious you want to be. I'm wondering if an option along the lines of --strict might make sense which turns on the options that the conservative people would want.

[devsnek]

@mhdawson i'm absolutely in favor of a flag to enable things like this. i just don't think throwing by default is a good choice for node or a good debugging experience, or a proper response of the larger issue, which is that node builtin apis are unaware of promises.

[mcollina]

@mcollina maybe we should update our server api to be aware of promises, not brick valid js programs.

Yes, definitely. I'm lacking good ideas on how to do so. The only way that could potentially work is to have selective behavior: if a promise is started from a server async context, a rejection will be automatically caught and it will cause the response to automatically return a 500.

I do not know how to implement that without breaking all of Node.js server usage or reduce Node.js throughput by at least a factor of 2. Have you (or somebody else) got any ideas on how to do so?

As things are now, the safest behavior is to crash on unhandledRejection. Removing the deprecation is a signal that unhandledRejection is fine and in good conscience I cannot approve this.

[jasnell]

@ljharb

That you can construct code that benefits from crashing is not sufficient to decide anything;

That's not what @mcollina is saying. The point is that it's trivial in Node.js to accidentally construct code with memory leaks and other bad things when we don't crash by default. That's not a hypothetical issue, we see this in the field all of the time.

@devsnek

maybe we should update our server api to be aware of promises, not brick valid js programs.

A noble goal, but not generally practical. As @mcollina points out, such changes would either break existing Node.js server code or yield a major performance regression (that's also not hypothetical... we've tested it). At this point, it's not clear how to achieve what you suggest here.

The issue here is fairly straightforward: Unhandled errors in a Node.js application are significantly less safe than unhandled errors in the browser. Less safe because they have real ramifications on available system resources, security, stability, and performance. This is not something we can simply shrug off and say, "Oh Well, JavaScript".

[devsnek]

@mcollina In the case of http, i'd add a createServerPromise or something which does handler(request, response).catch(do500). There are server libraries on npm today that use promises and i don't really see performance complaints about them. With fs, adding fs/promises decreases the cases where people create their own promise interfaces which could leak FDs.

Imagine if every function call where the first argument was an error object and the second was null was tracked to see if that error was dealt with in some way and if it wasn't, node crashed.

It's very common for (errorObject, null) to be a nodeback. I'd wager than >95% of function calls like that are nodebacks. But that doesn't mean we know for a fact that they are, and we have no way of knowing the actual intention of that function call.

When the function actually interacts with node builtins, we know it is a nodeback, and we can treat it as such. In the same way, promises that interact with node builtins can be held to a certain api contract (rejection is exception), while the rest can be left alone.

Just as you can't in good conscience allow these bugs and whatnot, i can't in good conscience allow node to trample the general promise api. I feel like there can be a compromise here.

[ljharb]

@jasnell yet violating the semantics of the language is not something that can be easily shrugged off with "oh, well, node". Promise.reject(); should not crash a node program, ever.

[jasnell]

should not crash a node program, ever

And by default, right now, it doesn't. We don't yet have a clear consensus on whether/when that's going to change, which is why removing the deprecation warning right now is premature without a decision from the @nodejs/tsc.

[ljharb]

@jasnell imo that's why having the deprecation warning is premature; the warning should only exist if there is consensus to make that change by default.

[jasnell]

The existing consensus of the TSC was to have the deprecation warning there. It wasn't added by accident or without TSC approval. The TSC needs to decide if that prior decision should be changed.

[jasnell]

There are server libraries on npm today that use promises and i don't really see performance complaints about them.

We (@mcollina and I) have.

This is informative.

With fs, adding fs/promises decreases the cases where people create their own promise interfaces which could leak FDs.

That is because I specifically implemented the underlying FileDescriptor class to automatically close the underlying fd on garbage collection... and even then that emits a warning because the best practice is still to manually close the FileDescriptor explicitly as soon as you're done with it to free up the fd.

That said, if you compare the performance of the fs.promises implementation to the core implementation, you'll find that fs.promises can be significantly slower.

[misterdjules]

@ljharb

yet violating the semantics of the language is not something that can be easily shrugged off with "oh, well, node". Promise.reject(); should not crash a node program, ever.

This is quite possibly out of scope for this PR but somehow I feel now might be a good time for me to share my thoughts about this.

I don't think people here shrug anything off. As @mcollina and @jasnell mentioned, they run into real-world examples of users of promises with Node.js having business impacting issues because the promises' error handling model doesn't necessarily fit their mental model and/or node's model. I frequently do run into those examples too.

I have the impression that very often this tension is framed as an "us vs them" problem with some Node.js users/maintainers on one side and JS language folks on the other side. I would love if instead this was discussed as a developers experience problem where the JS language and Node.js folks would engage in discussions that are more productive and constructive than what I've seen so far (not saying productive discussions don't happen, just that I haven't seen them).

Having a forum for Node.js users/maintainers and JS language folks to discuss concrete proposals of how the language and/or Node.js could evolve in order to fit those use cases seems like it would be more productive. Does such a thing exist? If not, how can we make that happen?

[BridgeAR]

I am closing this due to the concerns raised about removing the warning and since there was no further activity for ten month.

[mmarchini]

TSC will be voting on the intended default behavior for unhandled rejections on v15, where we also intend to remove the deprecation warning. We want to listen to Node.js users experiences with unhandled promises and what you think should be the default before voting, so we prepared a survey: https://www.surveymonkey.com/r/FTJM7YD

We also wrote an accompanying blog post for extra context: https://medium.com/@nodejs/node-js-promise-reject-use-case-survey-98e3328340c9

The survey will run for at least two weeks, at which point we'll evaluate if the number of replies is enough for us to move forward, otherwise we might extend it for a week or two. Please fill out the survey as it will help us decide the future of unhandled promise rejections on Node.js!