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

regression: UnhandledPromiseRejection is not fired until process is terminated #24209

Closed
aslushnikov opened this issue Nov 6, 2018 · 10 comments
Labels
promises Issues and PRs related to ECMAScript promises.

Comments

@aslushnikov
Copy link
Contributor

  • Version: v9.11.2, v10.11.0 and v11.1.0
  • Platform: x86_64 GNU/Linux

Repro

  1. save this is script.js:
Promise.reject(1);
setTimeout(() => {}, 2000);
  1. run node script.js

Expected: the UnhandledPromiseRejection is reported as soon as it happens; the node.js process
exits after 2 seconds.
Actual: nothing happens for 2 seconds, then the node.js process exits and only then prints UnhandledPromieRejection.

This works as expected in v8.12.0

@aslushnikov
Copy link
Contributor Author

(+cc @apapirovski who seems to be working on a related stuff)

@alexkozy
Copy link
Member

alexkozy commented Nov 6, 2018

It was regressed by following PR: #18207

@aslushnikov
Copy link
Contributor Author

Any updates on this? This is really unfortunate since scripts just hang in inconsistent state without letting users know that something's wrong.

@vsemozhetbyt
Copy link
Contributor

maybe cc @nodejs/promises-debugging

@targos targos added the promises Issues and PRs related to ECMAScript promises. label Nov 11, 2018
@Trott
Copy link
Member

Trott commented Nov 14, 2018

/ping @apapirovski

@Trott
Copy link
Member

Trott commented Nov 24, 2018

re-pinging @nodejs/promises-debugging

Is this a known issue? Works-as-expected/not-a-bug? Confirmed bug? Something else?

@BridgeAR
Copy link
Member

BridgeAR commented Nov 24, 2018

AFAIK this is done to prevent the possibility of an infinite recursion inside of the unhandled rejection listener.

Ideally it would trigger the warning as soon as possible (as in: after a single tick). To prevent the recursion from being possible, that's not an option though (it should actually be possible for the case that no such listener is attached, since then there is no possibility for the recursion in the first place but this would result in different timings with the listener attached and without). However, we are able to trigger the warning on setImmediate / right after the microtask queue is exhausted. It is already implemented to trigger on queue exhaustion and therefore I guess it's a bug in the implementation for cases like these.

I am trying to look further into it but I am not that familiar with the microtask code.

@apapirovski
Copy link
Member

The summary of this bug isn't quite correct. unhandledRejection was always fired as expected with the right timing. The only thing that didn't happen was logging the warning in case there was no listener for unhandledRejection. Fix is in #24632 but IMO this is a pretty insignificant bug.

@aslushnikov
Copy link
Contributor Author

The only thing that didn't happen was logging the warning in case there was no listener for unhandledRejection. Fix is in #24632 but IMO this is a pretty insignificant bug.

Oftentimes program transitions into inconsistent state after throwing an exception and hangs forever; you never know if it's just slow fetching resources from the Web or it has thrown an exception until you terminate it.

Thank you @apapirovski for handling this ❤️

Trott pushed a commit to Trott/io.js that referenced this issue Nov 28, 2018
PR-URL: nodejs#24632
Fixes: nodejs#24209
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 28, 2018

Fixed in 3ce9305

@Trott Trott closed this as completed Nov 28, 2018
targos pushed a commit that referenced this issue Nov 28, 2018
PR-URL: #24632
Fixes: #24209
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
PR-URL: nodejs#24632
Fixes: nodejs#24209
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 12, 2019
PR-URL: #24632
Fixes: #24209
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
rvagg pushed a commit that referenced this issue Feb 28, 2019
PR-URL: #24632
Fixes: #24209
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@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.

7 participants