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

Throw within queueMicrotask callbacks should not crash Node #38145

Open
Huxpro opened this issue Apr 8, 2021 · 8 comments
Open

Throw within queueMicrotask callbacks should not crash Node #38145

Huxpro opened this issue Apr 8, 2021 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem. repl Issues and PRs related to the REPL subsystem.

Comments

@Huxpro
Copy link

Huxpro commented Apr 8, 2021

  • Version:v15.9.0
  • Platform: macOS x86
  • Subsystem: task_queues

What steps will reproduce the bug?

λ node --trace-uncaught
Welcome to Node.js v15.9.0.
Type ".help" for more information.
> queueMicrotask(_ => { throw 1 });
undefined
>
node:internal/process/task_queues:94
    runMicrotasks();
    ^
1
Thrown at:
    at processTicksAndRejections (node:internal/process/task_queues:94:5)

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

If I understood the spec correct:

The queueMicrotask(callback) method must queue a microtask to invoke callback, and if callback throws an exception, report the exception.

It should behave similarly as Timer tasks.

> setTimeout(_ => {throw 1});
Timeout {
  ...
}
> Uncaught 1

What do you see instead?

Node crashed

Additional information

I did some preliminary triages that hopefully helps a bit:

  • task_queues.js invoke runMicrotask
  • which is a JS binding from node_task_queue.cc which invokes V8's PerformCheckpoint under the hood.
  • V8's PerformCheckpoint invoked V8's RunMicrotasks to actually evaluate those JS callbacks.

But since V8's PerformCheckpoint returns void. I'm not sure how can Node be aware of any exceptions threw during the evaluation of those JS callbacks. Chromium seems to handle this fine but I haven't got time looking at its source.

@Huxpro Huxpro changed the title Throw in the queueMicrotask callbacks Throw within queueMicrotask callbacks should not crash Node Apr 8, 2021
@Ayase-252 Ayase-252 added the process Issues and PRs related to the process subsystem. label Apr 8, 2021
@devsnek
Copy link
Member

devsnek commented Apr 8, 2021

The exception is reported via the uncaughtException event. If nothing handles that, the exception is fatal, like all other uncaught exceptions.

@Huxpro
Copy link
Author

Huxpro commented Apr 8, 2021

@devsnek ah that make sense.

Out of line question: there isn't an equivalent API on the Web that can do what process.on('uncaughtException', ..) on Node, right?

@legendecas
Copy link
Member

@Huxpro: there isn't an equivalent API on the Web that can do what process.on('uncaughtException', ..) on Node, right?

If you mean to monitor those uncaught exceptions, try window.addEventListener('error', ...) (and unhandledrejection for unhandledRejection in Node.js respectively).

@devsnek devsnek closed this as completed Apr 8, 2021
@Huxpro
Copy link
Author

Huxpro commented Apr 8, 2021

@devsnek although it's spec-conformant in that sense. Why wouldn't queueMicrotask behave the same as setTimeout?

@targos
Copy link
Member

targos commented Apr 11, 2021

I'm reopening because it seems worth fixing (if possible) in the REPL.

@targos targos reopened this Apr 11, 2021
@targos targos added the repl Issues and PRs related to the REPL subsystem. label Apr 11, 2021
@Linkgoron
Copy link
Member

I'm reopening because it seems worth fixing (if possible) in the REPL.

This actually works in the REPL of Node 12 and 14.4 but stopped working in Node 14.5. I think that the issue is that errors thrown in queueMicroTask don't get captured by the domain, and don't reach the domain error handler (in the repl). The behaviour change was probably caused by this change #33859

@EladKeyshawn
Copy link

@Linkgoron Does it still need fixing?

@BridgeAR
Copy link
Member

@EladKeyshawn yes, it does.

The question is how something like that might be solved. One way would be to use child processes to execute the code and to detect fatal exceptions leading to a new child process. However, that would only recover the REPL but all former input would be lost. Thus, we would have to print a clear warning about what happened. This is the best solution I can think of and it would also resolve multiple other reports.

@BridgeAR BridgeAR added the confirmed-bug Issues with confirmed bugs. label Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. process Issues and PRs related to the process subsystem. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants