Skip to content
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

unhandledRejection falls into infinite recursion #17913

Closed
eduardbme opened this issue Dec 29, 2017 · 14 comments
Closed

unhandledRejection falls into infinite recursion #17913

eduardbme opened this issue Dec 29, 2017 · 14 comments
Labels
promises Issues and PRs related to ECMAScript promises.

Comments

@eduardbme
Copy link
Contributor

eduardbme commented Dec 29, 2017

  • Version: 8+ (probably all versions that supports unhandledRejection event)
  • Platform: linux
  • Subsystem: process

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 unhandledRejection allows you to fall into infinite recursion if code within handler creates unhandled promise rejection ? I expect the same behavior as for uncaughtException (exit process with non-zero code).

unhandledRejection handler 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:

'use strict';

process.on('unhandledRejection', uncaughtRejection => {
    console.log('unhandledRejection handler catch: ' + uncaughtRejection);

    Promise.reject(3);
});

function foo() {
    return Promise.reject(1);
}

foo();

Thanks in advance.

@eduardbme eduardbme changed the title unhandledRejection fall into infinite recursion unhandledRejection falls into infinite recursion Dec 29, 2017
@eduardbme
Copy link
Contributor Author

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.

@bnoordhuis
Copy link
Member

bnoordhuis commented Dec 29, 2017

Working as intended, IMO. Promise.reject(3) doesn't throw an exception, it creates a promise that triggers a new unhandledRejection event on the next tick. There is no recursion, just repeated unhandledRejection events.

If you replace it with a throw 'boom', you get the behavior you expect. Replace it with Promise.reject(3).catch(console.error) and you get only a single unhandledRejection event.

@bnoordhuis bnoordhuis added the promises Issues and PRs related to ECMAScript promises. label Dec 29, 2017
@eduardbme
Copy link
Contributor Author

eduardbme commented Dec 29, 2017

Okay, 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 ?

@eduardbme
Copy link
Contributor Author

eduardbme commented Dec 29, 2017

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.

@apapirovski
Copy link
Member

@eduardbcom Since unhandledRejection happens on nextTick, there's no actual infinite loop. The other code in your app can still run.

@eduardbme
Copy link
Contributor Author

@apapirovski

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.

@eduardbme
Copy link
Contributor Author

eduardbme commented Dec 29, 2017

As a plus sign, I don't see any advantage in current implementation.
If after unhandledRejection callback execution you have one another unhandledRejection there 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 :)

@apapirovski
Copy link
Member

@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.

@eduardbme
Copy link
Contributor Author

eduardbme commented Dec 29, 2017

@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 unhandledRejection several times during program execution, but IMO it's not okay to emit this event after processing callback for that event..

@apapirovski
Copy link
Member

@eduardbcom I think that we should conform to the expected async behaviour of promises. The problem is that the current code that emits unhandledRejection inadvertently creates an infinite loop — but it probably shouldn't.

@bnoordhuis I think we need to at least allow the nextTick loop to proceed but I could see an argument for even allowing the nextTick loop to finish and only invoke any new promise rejection on the next run of _tickCallback. Thoughts? This is the problematic code:

function emitPendingUnhandledRejections() {
let hadListeners = false;
while (pendingUnhandledRejections.length > 0) {
const promise = pendingUnhandledRejections.shift();
const reason = pendingUnhandledRejections.shift();
if (hasBeenNotifiedProperty.get(promise) === false) {
hasBeenNotifiedProperty.set(promise, true);
const uid = promiseToGuidProperty.get(promise);
if (!process.emit('unhandledRejection', reason, promise)) {
emitWarning(uid, reason);
} else {
hadListeners = true;
}
}
}
return hadListeners;
}

As you can see, the pendingUnhandledRejections can just get refilled with new rejections while we're iterating over it. That seems problematic to me.

@eduardbme
Copy link
Contributor Author

Can fix it, but we need to reach an agreement of the expected behavior.

@bnoordhuis
Copy link
Member

Something like this is acceptable, IMO:

const rejections = pendingUnhandledRejections;
pendingUnhandledRejections = [];
while (rejections.length > 0) {
  // ...
}

(And might as well change to that a for loop 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.

@eduardbme
Copy link
Contributor Author

the plan is to make unhandled rejections fatal errors anyway

Even in the case we listen that event ?

@mcollina
Copy link
Member

mcollina commented Jan 3, 2018

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.

addaleax pushed a commit to addaleax/node that referenced this issue Feb 27, 2018
Remove the unnecessary microTasksTickObject for scheduling microtasks
and instead use TickInfo to keep track of whether promise rejections
exist that need to be emitted. Consequently allow the microtasks to
execute on average fewer times, in more predictable manner than
previously.

Simplify unhandled & handled rejection tracking to do more in C++ to
avoid needing to expose additional info in JS.

When new unhandledRejections are emitted within an unhandledRejection
handler, allow the event loop to proceed first instead. This means
that if the end-user code handles all promise rejections on nextTick,
rejections within unhandledRejection now won't spiral into an infinite
loop.

PR-URL: nodejs#18207
Fixes: nodejs#17913
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Remove the unnecessary microTasksTickObject for scheduling microtasks
and instead use TickInfo to keep track of whether promise rejections
exist that need to be emitted. Consequently allow the microtasks to
execute on average fewer times, in more predictable manner than
previously.

Simplify unhandled & handled rejection tracking to do more in C++ to
avoid needing to expose additional info in JS.

When new unhandledRejections are emitted within an unhandledRejection
handler, allow the event loop to proceed first instead. This means
that if the end-user code handles all promise rejections on nextTick,
rejections within unhandledRejection now won't spiral into an infinite
loop.

PR-URL: nodejs#18207
Fixes: nodejs#17913
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants