Repository navigation
unhandledRejection falls into infinite recursion #17913
Description
Activity
- changed the title
[-]unhandledRejection fall into infinite recursion[/-][+]unhandledRejection falls into infinite recursion[/+]on Dec 29, 2017 I propose to emit warning about promise unhandled rejection on the second recursion call, and exit process with error code, probably some special code to indicate specificity of this exit event.
I don't expect that any code rely on this behavior, so I assume that it's safe to fix it.
May prepare Christmas fix for that.Working as intended, IMO.
Promise.reject(3)doesn't throw an exception, it creates a promise that triggers a newunhandledRejectionevent on the next tick. There is no recursion, just repeatedunhandledRejectionevents.If you replace it with a
throw 'boom', you get the behavior you expect. Replace it withPromise.reject(3).catch(console.error)and you get only a singleunhandledRejectionevent.Reacted by Benjamin GruenbaumOkay, let me show you some another code.
'use strict'; process.on('unhandledRejection', uncaughtRejection => { console.log('unhandledRejection handler catch: ' + uncaughtRejection); logger.error('log some error'); // 3-d party logger }); function foo() { return Promise.reject(1); } foo(); // somewhere in logger module // code that you don't control logger = { error() { Promise.reject(1); // sometimes it happens.. } }you got the idea ?
There is no recursion, just repeated unhandledRejection events.
Why we cannot say the same thing about uncaughtException. "There is no recursion, just repeated uncaughtException events." ?
But uncaughtException IMO has correct behavior avoiding infinite recursion. The same should have unhandledRejection.
@eduardbcom Since
unhandledRejectionhappens onnextTick, there's no actual infinite loop. The other code in your app can still run.By saying infinite recursion I mean that we infinitely entering that function. And there is a case when no another code would be run cuz user's micro function can completely fill up promise micro stack.
As a plus sign, I don't see any advantage in current implementation.
If afterunhandledRejectioncallback execution you have one anotherunhandledRejectionthere is a chance (a big change) that you will have it infinitely. And as a result side effect (infinite functions call, whatever you call it).Any reason to love this behavior ?
@apapirovski that was not working code :)
@eduardbcom After looking at the code, I don't think the current behaviour is intended. Pretty sure it was meant to let the event loop proceed first.
@apapirovski but you also don't like the idea about process.exit on the second sequential call ?
don't get me wrong, its okay to emit
unhandledRejectionseveral times during program execution, but IMO it's not okay to emit this event after processing callback for that event..@eduardbcom I think that we should conform to the expected async behaviour of promises. The problem is that the current code that emits
unhandledRejectioninadvertently creates an infinite loop — but it probably shouldn't.@bnoordhuis I think we need to at least allow the
nextTickloop to proceed but I could see an argument for even allowing thenextTickloop to finish and only invoke any new promise rejection on the next run of_tickCallback. Thoughts? This is the problematic code:node/lib/internal/process/promises.js
Lines 100 to 116 in 4117e22
As you can see, the
pendingUnhandledRejectionscan just get refilled with new rejections while we're iterating over it. That seems problematic to me.Reacted by Eduard BondarenkoCan fix it, but we need to reach an agreement of the expected behavior.
Something like this is acceptable, IMO:
const rejections = pendingUnhandledRejections; pendingUnhandledRejections = []; while (rejections.length > 0) { // ... }
(And might as well change to that a
forloop for efficiency:)for (let i = 0; i < rejections.length; i += 2) { const promise = rejections[i + 0]; const reason = rejections[i + 1]; // ... }
Anything more complex is probably not worth it because it's an edge case and the plan is to make unhandled rejections fatal errors anyway.
the plan is to make unhandled rejections fatal errors anyway
Even in the case we listen that event ?
I think the expected behavior is for them to be fatal. However, it is very hard to reach an agreement on the matter. I have a polyfill for that behavior if you want to try that out: https://github.com/mcollina/make-promises-safe.
I think this is too much of an edge case and a programmer error: I do not think it's Node.js responsibility to handle.
- added a commit that references this issue
on Feb 27, 2018 - added a commit that references this issue
on May 8, 2018 - added a commit that references this issue
on Jul 27, 2026
unhandledRejectionevent)According to documentation of
uncaughtException- 'Exceptions thrown from within the event handler will not be caught. Instead the process will exit with a non-zero exit code and the stack trace will be printed. This is to avoid infinite recursion.'.So why
unhandledRejectionallows you to fall into infinite recursion if code within handler creates unhandled promise rejection ? I expect the same behavior as foruncaughtException(exit process with non-zero code).unhandledRejectionhandler may contain some logic to log that situation, and owing to the fact that 3-d party logger implementation can produce unhandled rejection within himself , we can have infinite recursion.Code example to reproduce:
Thanks in advance.