-
Notifications
You must be signed in to change notification settings - Fork 0
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
Deferred rejection #9
Comments
Not necessarily because you don't necessarily know when a promise rejection will be handled. One example is TBH I think this is really just a problem with node that node itself needs to fix - warning about unhandled rejections by default is silly. It's handy as an educational tool but I really don't think node's primary purpose is to teach kids how to code. |
Good point. Sometimes you want to wait forever. |
Yeah doesn't the halting problem suck? Mostly, this whole mess just gives me more reason to switch over to using observables exclusively. It's pretty pitiful that node considers creating a really hacky global stream (term used loosely) of unhandled rejections a major feature, life is much easier when you just have an observable of observables (or an observable of promises). |
Interesting idea, but who is responsible for that? The producer (who creates the promise) or the consumer (who awaits it)? It seems it can't be the producer, as he doesn't know for how long the asynchronous attachment should be allowed. |
Nah, should definitely be the consumer. Sure they could forget, but there's always ways to misuse an API and trying to come up with every possible mistake someone can make is a Sisyphean task. More than enough tools out there, co, rxjs, xstream, etc., for globbing together promises to consolidate error handling, it's a waste for core node to add yet another tool. |
The consumer typically. It's a halting problem as @vkarpov15 said so nothing else to do. Software can't decide it so the programmer has to. They just have to do it carefully. If someone explicitly says "I'm prepared to handle this" I think it's reasonable to believe them and just assume they're competent to do it. Maybe unhandled rejections should still be delivered on garbage collection though ? The batch helps. It taints the entire tree of derived promises to any depth. Then you can end deferral for the entire batch in one place. |
My use case that led to this is kind of a middleware. I run a procedure provided by the user and give it the ability to start certain asynchronous subprocesses. The procedure may be a generator or async function. It's easy and seemingly valid from a coder point of view to do this:
If that first subprocess is rejected during step 2 you get an unhandled rejection (and soon in node a crash). I wanted to just give them the chance to attach before procedure end. But there is a definite end to this so after procedure ends I also want to deliver anything they genuinely left unhandled. The typical advice here is to add a handler to suppress the unhandled rejection. That doesn't work. I've got to know at the end if they didn't handle it. There has to be a period where it's valid, ended by a point where it becomes invalid. |
If it's the consumer, then this should be an opt-in. For example: const promise = doSomething();
const release = promise.supressUnhandledRejection();
// later
promise.then(…).catch(handleErr);
// afterwards
release(); // emits the unhandled rejection event if there was a rejection and it still hasn't been handled However, you can already do that! Admittedly, you don't have access to the internal const promise = doSomething();
let rejectionHandled = true; promise.catch(() => rejectionHandled = false; });
// later
promise.then(…).catch(handleErr);
rejectionHandled = true;
// afterwards
if (!rejectionHandled) process.emit("unhandledRejection", promise); // or whatever @icysailor It sounds like you are looking for |
Agreed about the opt in. Was thinking of it as a separate constructor. But maybe a static method would work. So A devoted constructor would enable the full set of methods though. So you could do It's true that you could manage it externally. But then you have to manage it. It's easier if your workflow is just:
|
My example above was poor. I was thinking of the scenario where the rejection is handled and so shouldn't be delivered - but the handling happens a tick or 2 after the rejection. async function myProcedure () {
const process1 = doThis(); // Fire this off to get it running
const process2 = doThat();
const process2Result = await process2;
/*** CRASH from process1 - even though I've handled it 2 lines down */
try {
const process1Result = await process1;
// .. do something with process1Result
} catch (e) {
// Do nothing because a process1 failure is not a problem
}
} This seems perfectly reasonable to do. It would never complain in a synchronous function. Yet here it crashes the entire program. Maybe this is really about async functions. This really hurts the goal of async functions: make it possible to write an async algorithm the same way you would write a synchronous one. It forces the coder to break this into a sequence of steps with boundaries between them (back to the way we had to think of it before). Maybe async functions should defer unhandled rejection of promises they can access. From the coder's perspective the procedure-that-may-handle-the-rejection is still running. It should have the same effect on rejections that a synchronous function does: it may still be handled so wait and see what happens. |
Yeah, an extra promise constructor could do. However, I still think it's unnecessary (at least to be included in the standard, as you demonstrated you already can achieve this by subclassing). |
Regarding your new example, you cannot really compare asynchronous non-sequential code to synchronous code. If this was a synchronous function, your program would indeed crash as soon as
async function myProcedure () {
const process1 = doThis().catch(e => {
// Do nothing because a process1 failure is not a problem
});
const process2 = doThat();
const [process1Result, process2Result] = await Promise.all([process1, process2]);
if (process1Result != null) {
// .. do something with process1Result
}
} |
I think the Node team needs to be more clear on what their target demographic is before a reasonable decision can be made on how to proceed on this subject. If you (node maintainers) are trying to make something that is "hard to get wrong" for beginners then built-in unhandled rejection warnings/crashing is a reasonable approach. If you are trying to build something that makes it easy for senior engineers to build advanced complex performance critical applications, then built-in unhandled rejection warnings/crashings is not a reasonable approach. As has been mentioned previously in this thread and others, in advanced applications and libraries it is reasonable and correct to not know when a promise will be handled and it is reasonable and correct for a promise to never be handled! There is no solution that solves both use cases of "easy for beginners" and "easy for experts" because they are in opposition to each other. Personally, I have always struggled to see JS (and therefore node) as a reasonable language/platform for building advanced applications on due to reasons that are well documented all over the internet. Unfortunately, there aren't any real alternatives in the browser at this time so we are stuck using a language that feels like it is built/designed for beginners (especially with decisions like this) to build large complex applications. |
@MicahZoltu It's never correct not to handle possible errors from a promise, except when you know that a promise will never be rejected. |
@bergus You're right, it's different. You never start a procedure on one line and get the result on another line in a normal function. Whatever operation you called is done when the call returns. |
@bergus so you admit you don't necessarily know when a rejection will get handled, but you claim that you always need a |
@vkarpov15 Any unclosed promise chain is a cause of bugs (either unhandled errors or forgotton asynchrony), so yes, you always need a A beginner should rarely find himself in such a case. It's an expert scenario. The usual developer will want to treat errors immediately. And no, when you |
I think pedantry wastes my time as an engineer. If I wanted a language that prevented me from doing any meaningful work I'd go back to writing Java. |
I don't want to rehash the debates that occurred in the other threads too much, but the short version is that a fire-and-forget async operation is a reasonable use case but the library that is creating/providing the promise can't know how it will be used ahead of time. As an example, if I am opportunistically writing data to a remote cache I don't need to do any work when it is done. As a user of a library, I will call something like The other harder to address use case is when you want to pre-warm some data in your application. For example, you may kick off an async request to a remote resource while the user is loading the page, because you suspect that they may later click a UI element that requires the data from that resource. Because you are pre-fetching data, there is no appropriate place to display the error to the user as the user hasn't asked for the data yet. In this case, you will never attach a rejection handler if the user never requests the data, or you may attach a rejection handler at some distant point in the future, after the user has actually requested it. In this case, it would be incorrect to opportunistically attach a rejection handler before the user requests the data because you need to not handle the error so if the user does ask for the resource you can attach a handler which would immediately fire the exception handling (assuming the promise was already fulfilled at that time). There are workarounds for these scenarios, like storing the promise in a variable and then attaching a catch to that variable and then returning the original variable. However, in promise heavy code this very quickly destroys readability as now every line of code can turn into a mess of unhandled rejection disabling. This is why I argue that the Node maintainers need to decide whether they want to support the trivial use case or support complex promise heavy applications that do things like fire-and-forget, pre-fetching, lots of promises being passed around, etc. I recognize that promise based asynchronous programming is a different paradigm than what people are used to with synchronous programming, but it can be an incredibly powerful tool if wielded well. The unhandled rejection stuff is catering to an audience that isn't wielding it to its full potential and are stuck thinking of their code as synchronous code, which its not. |
Great points 👍 |
Hi folks. Interested in feedback on some experimental promises with deferred rejection. Has this been considered as a solution?
The pattern is you create promises with rejection deferred: if they're rejected without a reject handler it will be postponed for later delivery. If a reject handler is added later the rejection is delivered. At the point when you expect your process to have handled the promise, you end the deferral which delivers any still-pending rejections as unhandled.
This makes it the responsibility of the programmer to specify when a rejection is considered unhandled. If you have a use for asynchronously-attached handlers you can do that without the engine getting in the way and without breaking other code by changing global behavior. But you don't completely lose error reporting because you still deliver anything unexpected at the end. You create little pockets of time where asynchronous attachment is valid.
There's a working implementation here. Usage and implementation are simple. It subclasses native Promises:
https://github.com/icysailor/promise-deferred-rejection
@MicahZoltu @vkarpov15 Would this work for the advanced use cases you guys have mentioned?
Relevant to the discussion in #6 nodejs/node#830 nodejs/promises#26 https://gist.github.com/benjamingr/0237932cee84712951a2
The text was updated successfully, but these errors were encountered: