-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
opt-out or opt-in for warnings etc. #22587
Comments
FWIW both of these PRs are opt-in. On a larger note, it is not Node's job to judge your code. Node.js's job is to run your code. What Node.js can instead do is provide channels to acquire information that tooling can use to judge your code. As an example: As another example: We should be able to recognize anti-patterns and provide details about them without judging. We should only provide channels of data when we have 100% correct information. |
I think we should provide sane, configurable defaults that avoid the most common pitfalls of using Node.js. |
I think it would be useful to break this down just a bit differently. Promises running in the browser vs. Promises running in Node.js have a number of distinct differences. If I am writing bad code that leaks memory because I fail to handle a rejection (intentionally or otherwise), that has less of an impact in the browser than it does in Node.js. That is, with Node.js, failure to properly dispose of resources in failure conditions can have far more damaging impact on the entire system. Further, improper use of Promises is one of the single most common programming mistakes made by Node.js developers to date. In every single consulting engagement we have with customers we end up dealing with this issue. It's to the point where it's one of the very first questions we ask and it's one of the very first things we look for when helping to evaluate any issues the customer may be having. What makes this conversation difficult is the fact that coding patterns that are perfectly within the design of the Promises API make it trivial to introduce bugs into Node.js applications directly because of the design choices that have been made in Node.js that are different than browsers. It is precisely because of these design choices that I believe it is responsible of Node.js to proactively provide reasonable feedback to users when problematic conditions are detected. @devsnek, you said:
That is true to an extent. But providing a warning about an unhandled rejection that could be causing resources to leak is not a judgement of code quality, it's an indication that there could be a real programming bug in the code that is running. C/C++ compilers do this all the time with providing compile time warnings about loss of precision in casts, variables that are unused, and so forth. With JavaScript, we don't really have the ability to reliably detect these kinds of problematic conditions via static code analysis and linting, making run-time warnings when they actually occur the only reliable mechanism. It's not ideal, but it's what we have. There is already precedence in Node.js for these kinds of things, for example:
With regards to the two PRs in question... there are two separate issues to contend with:
Let's not lose the fact that we need answers to both of these points. |
If our run-time warnings were reliable I would be more on board with this. Seeing suggestions like exiting on gc gives me a knee-jerk reaction because those methods aren't reliable. If we can't be reliable and correct, there's no point in the warning in the first place, people will just ignore/disable it. |
@devsnek I know, right? Still, twice I was told that the big question that needed answering wasn't which PR to go with, but the larger meta-issue of opt-in vs opt-out. So here we are. /pinging @benjamingr for more context here... |
@jasnell Yes they both need answering, but only one needs answering in this issue. Or at least that was my hope. I opened this issue specifically to separate the question of correct general approach to warning the user (this issue, and something both PRs do the same way currently) from preferable API (difference between those two PRs, not addressed here). I think the context you provide is helpful, but I do want to keep away from scope creep if possible. (I concede in advance that it may not be possible...) |
@devsnek So even if Node.js as a project decides that the right path is "Node.js should attempt to be as useful as possible and ask people to opt-out", we still ideally might not want the exit-on-unhandled-rejection stuff (coming in the future, at least according to the warning that is now emitted) because the issue there is that it isn't 100% reliable that the rejection won't be handled later and therefore it may not be a useful/good defaults in your view? Am I understanding correctly? It does seem like the general feedback from users has been positive about the warn-on-unhandled-rejection stuff, suggesting they have found it helpful, but I don't have any direct knowledge of that and am certainly open to contrary information. /cc @nodejs/promises-debugging |
I believe that if Node.js treats promise rejections as non-fatal it should also treat exceptions as non-fatal and that the behaviour for unhandled rejections should be the same as the behaviour for uncaught exceptions. Otherwise, things are very quirky: db.read('foo', (err, data) => {
doSomething(JSON.parse(data)); // throws on invalid JSON terminating the server
db.release();
}); The above code throws and terminates the process. The below code: async function doFoo() {
const data = await db.read('foo');
doSomething(JSON.parse(data)); // throws on invalid JSON terminating the server
db.release();
}); Does not crash the process but instead logs a warning (used to silent error before 7.6). Even just adding the keyword
Absolutely, the warnings are not contested by Gus and he's for them as far as I know. The question is about whether or not terminating on unhandled rejections (detected some way) is a good idea. No matter what we do there is no 100% perfect solution - the question is whether or not it's better to prefer theoretical correctness (we can't do it always so we should never do it) vs. earlier errors (we should be as helpful as possible towards our users). |
Worth mentioning exit on GC does not have any false negatives (that is, whenever it exits an uncaught exception has 100% occured). It does have false positives (which you still get a warning for). The question is in a gist: Should we try to find as many exceptions as possible and exit on them or should we say that because we can't find all of them (because a user might have a memory leak) we shouldn't find any at all. Exit on GC used to give me a knee-jerk reaction as well 5 years ago. It still does (including to some promises-debugging members) ( cc @misterdjules ). The problem is that we have three camps (of opinions here, we're all friendly) here:
The good thing is that unlike the last few times we've had this discussion none of the participants are coming from a place of "we know what's the right thing to do here". I think all parties understand there are tradeoffs and no solution is perfect. |
Lastly, I'd like to echo @jasnell 's sentiment. I've personally spent the last few years working on several promise libraries (like bluebird) and answering (over 800 questions about it in Stack Overflow. People find asynchronous code incredibly challenging and make common mistakes. This is our fault as a platform and we could definitely do things better. A part of this is my fault due to personal issues. We have a survey we're planning to send but I will only be available for that after I'm back from my holiday on ~ Sep 30th. |
Another place where this philosophical question (useful/opt-out vs. fewest-assumptions/opt-in) might apply is with regard to Buffer constructor warning. It's possible that this isn't something that can be decided in a generalized way. It may need to be decided on a case-by-case basis. You need to weigh the pain felt by people not getting warnings when perhaps they should against the pain felt by people getting warnings when perhaps they shouldn't. Which is a bigger problem is likely to vary from feature to feature. |
It also differs, for example, because Buffer is node’s, but Promise is not - there’s a higher authority for language builtins. node has much greater flexibility with its own core types and modules than it should with language builtins, if it wants javascript to remain somewhat portable from and to node. |
In terms of the original question as to the philosophy: Opt-in versus opt-out. I don't think that either extreme is good for end users. We want options that are good for most end users to be opt-out and those that are more appropriate in special cases to be opt-in. Lots of configurable options sound good to developers (including me) because they provide lots of flexibility in case we run into problems. However, customer feedback over the years had been that they have a hard time finding the right combination if it is opt-in. They would generally prefer a set of default options which is "good enough" rather than having to spend a lot of time finding the options that are perfect for them. The net is that as @Trott mentioned earlier, it is most likely a decision that needs to be made on a case-by-case basis. |
At the TSC meeting today, there was no opposition to "this needs to be decided on a case-by-case basis". I've removed this from the That said, if what we really want is for the TSC to choose between these three approaches (below, copy/pasted from @benjamingr's comment/summary) for the specific case of unhandled Promise rejections, let's get this back on the agenda and we'll get that sorted, assuming all parties agree that further conversation (instead of escalating to the TSC) is unlikely to result in a compromise or a resolution to the (friendly but real and significant) disagreement:
|
My stance will continue to be "provide good informative defaults". Therefore, attempt to tell a user when something may have gone wrong, with an opt-out. |
I agree, but I'm only interested in providing data to the user when that data is correct. If we introduce incorrect data, users cannot trust any data that comes from node, and our data is made useless. With this regard, removing the warnings in console for "unhandled rejections" would coerce users to use the unhandledRejection and rejectionHandled events to better track state, since we can't erase things we put in the console. As for exit on GC, as long as it doesn't catch all the errors its quite literally useless, because the problem we're trying to solve is catching all the errors. I can turn it on but then what about all the errors it doesn't catch. Isn't that what we're trying to solve in the first place? I'm very confused about why people are pushing for this. |
Unfortunately determining if it's "correct" is not always possible. When it comes to handling potential failure situations, false positives are almost always better than deferring action because we're not sure. |
@jasnell I think if we can't make a correct decision, we should give users the ability to make their own decision, not make the decisions for them. |
I don’t agree that false positives are better - those undermine user trust in the system itself, leading them to disable, or worse, mentally ignore, future warnings. |
I'm also on the side that false positives are not always better. If there are too many people will just turn off a feature reducing its usefulness. I'd prefer that it only take hard action (for example throwing) in cases were we are sure and document that it might not cover all of the potential cases (because there are some we cannot be sure of). |
I'm going to close this, but feel free to keep commenting and/ore re-open if there are important things to say. It does seem like #22822 will handle the specific case about promises and unhandled rejections, and there is already an issue open for Buffer constructor. |
I've been trying to get my head around the real philosophical differences between #22218 & #21857 and it's still not entirely clear for me. But, @benjamingr's summary above is really helpful and I'd suggest anyone skimming this should review that. I would like to reinforce @jasnell's point above because it echoes what we experience when out in the field dealing with customers and problems they experience—these are average developers doing everyday types of things but suffering from language constructs which may be making async-code a slightly easier but introducing a heap of new avenues for errors in the process:
What I really want out of Node isn't opt-out warnings or anything intrusive, but really good ways of opting-in and/or inspecting behaviour which can suggest problems. So I quite like the basis of @BridgeAR's PR @ #22218 because it'd give us a tool to suggest to users to help them conform to "best practice". I understand the Promise purists wanting Node not to intrude into the huge flexibility afforded by Promises, but its this flexibility (and some other "features") that causes pain. So just let us have some tools so we can opt-in to sensible defaults. |
@nodejs/tsc:
Refs: #22218
Refs: #21857
@benjamingr @devsnek @BridgeAR
The text was updated successfully, but these errors were encountered: