async_hooks,worker: fix process.exit() crashes in async fn with async_hook enabled by addaleax · Pull Request #33347 · nodejs/node

addaleax · 2020-05-11T00:41:30Z

worker: call CancelTerminateExecution() before exiting Locker

As the comment indicates, this fixes a DCHECK failure, although I don’t
quite understand why it is happening in the first place.

async_hooks: clear async_id_stack for terminations in more places

Termination exceptions are similar to uncaught exceptions in that they
should clear the async id stack, because no ongoing async callbacks
will be brought to completion when execution terminates.

Previously, there was a check that made sure that that happened when
the termination occurred during the callback itself, but no such check
was in place for the case that the termination occurred during
microtasks started by them. This commit adds such a check, both for
microtasks and the nextTick queue. The latter addition doesn’t fix
a crash, but still makes sense conceptually.

The condition here is also flipped from applying only on Worker threads
to also applying on the main thread, and setting the failed_ flag
rather than reading it. The former makes sense because the public C++
Stop(env) API can have the same effect as worker thread termination,
but on the main thread rather than a Worker thread.

test: regression tests for async_hooks + Promise + Worker interaction

Add regression tests for the case in which an async_hook is enabled
inside a Worker thread and process.exit() is called during the
async part of an async function.

This commit includes multiple tests that seem like they should all
crash in a similar way, but interestingly don’t. In particular, it’s
surprising that the presence of a statement after process.exit()
in a function has an effect on the kind of crash that’s being
exhibited (V8 DCHECK vs. assertion in our own code) and the
circumstances under which it crashes (e.g. the -1 and -2 tests
can be “fixed” by reverting 13c5a16, although they
should have the same behavior as the -3 and -4 tests).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place.

Termination exceptions are similar to uncaught exceptions in that they should clear the async id stack, because no ongoing async callbacks will be brought to completion when execution terminates. Previously, there was a check that made sure that that happened when the termination occurred during the callback itself, but no such check was in place for the case that the termination occurred during microtasks started by them. This commit adds such a check, both for microtasks and the `nextTick` queue. The latter addition doesn’t fix a crash, but still makes sense conceptually. The condition here is also flipped from applying only on Worker threads to also applying on the main thread, and setting the `failed_` flag rather than reading it. The former makes sense because the public C++ `Stop(env)` API can have the same effect as worker thread termination, but on the main thread rather than a Worker thread.

Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a16, although they should have the same behavior as the -3 and -4 tests).

nodejs-github-bot · 2020-05-11T00:42:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31271/ (:yellow_heart:)

test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js

benjamingr · 2020-05-11T15:59:14Z

This is surprising, reading 13c5a16 I don't understand why it causes the test behavior change.

(The changes themselves in this PR look fine although I also don't understand why CancelTerminateExecution is needed)

…raction Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

addaleax · 2020-05-11T17:52:52Z

This is surprising, reading 13c5a16 I don't understand why it causes the test behavior change.

Me neither, really.

(The changes themselves in this PR look fine although I also don't understand why CancelTerminateExecution is needed)

Yeah, me neither. I think the reason that V8 aborts here is that some TryCatch catches the termination exception and doesn’t properly clean up after itself, but that seems weird too.

nodejs-github-bot · 2020-05-11T17:55:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31280/ (💛)

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Termination exceptions are similar to uncaught exceptions in that they should clear the async id stack, because no ongoing async callbacks will be brought to completion when execution terminates. Previously, there was a check that made sure that that happened when the termination occurred during the callback itself, but no such check was in place for the case that the termination occurred during microtasks started by them. This commit adds such a check, both for microtasks and the `nextTick` queue. The latter addition doesn’t fix a crash, but still makes sense conceptually. The condition here is also flipped from applying only on Worker threads to also applying on the main thread, and setting the `failed_` flag rather than reading it. The former makes sense because the public C++ `Stop(env)` API can have the same effect as worker thread termination, but on the main thread rather than a Worker thread. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a16, although they should have the same behavior as the -3 and -4 tests). PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

addaleax · 2020-05-15T17:38:43Z

Landed in c7eeef5...e65d189

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Termination exceptions are similar to uncaught exceptions in that they should clear the async id stack, because no ongoing async callbacks will be brought to completion when execution terminates. Previously, there was a check that made sure that that happened when the termination occurred during the callback itself, but no such check was in place for the case that the termination occurred during microtasks started by them. This commit adds such a check, both for microtasks and the `nextTick` queue. The latter addition doesn’t fix a crash, but still makes sense conceptually. The condition here is also flipped from applying only on Worker threads to also applying on the main thread, and setting the `failed_` flag rather than reading it. The former makes sense because the public C++ `Stop(env)` API can have the same effect as worker thread termination, but on the main thread rather than a Worker thread. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a16, although they should have the same behavior as the -3 and -4 tests). PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Termination exceptions are similar to uncaught exceptions in that they should clear the async id stack, because no ongoing async callbacks will be brought to completion when execution terminates. Previously, there was a check that made sure that that happened when the termination occurred during the callback itself, but no such check was in place for the case that the termination occurred during microtasks started by them. This commit adds such a check, both for microtasks and the `nextTick` queue. The latter addition doesn’t fix a crash, but still makes sense conceptually. The condition here is also flipped from applying only on Worker threads to also applying on the main thread, and setting the `failed_` flag rather than reading it. The former makes sense because the public C++ `Stop(env)` API can have the same effect as worker thread termination, but on the main thread rather than a Worker thread. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a16, although they should have the same behavior as the -3 and -4 tests). PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place. PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Termination exceptions are similar to uncaught exceptions in that they should clear the async id stack, because no ongoing async callbacks will be brought to completion when execution terminates. Previously, there was a check that made sure that that happened when the termination occurred during the callback itself, but no such check was in place for the case that the termination occurred during microtasks started by them. This commit adds such a check, both for microtasks and the `nextTick` queue. The latter addition doesn’t fix a crash, but still makes sense conceptually. The condition here is also flipped from applying only on Worker threads to also applying on the main thread, and setting the `failed_` flag rather than reading it. The former makes sense because the public C++ `Stop(env)` API can have the same effect as worker thread termination, but on the main thread rather than a Worker thread. PR-URL: nodejs#33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a16, although they should have the same behavior as the -3 and -4 tests). PR-URL: #33347 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>

nodejs/node#33347

addaleax added 3 commits May 11, 2020 01:24

worker: call CancelTerminateExecution() before exiting Locker

b523a31

As the comment indicates, this fixes a DCHECK failure, although I don’t quite understand why it is happening in the first place.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. worker Issues and PRs related to Worker support. labels May 11, 2020

addaleax added the async_hooks Issues and PRs related to the async hooks subsystem. label May 11, 2020

addaleax added the lts-watch-v12.x label May 11, 2020

cjihrig approved these changes May 11, 2020

View reviewed changes

test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js Outdated Show resolved Hide resolved

jasnell approved these changes May 11, 2020

View reviewed changes

benjamingr approved these changes May 11, 2020

View reviewed changes

fixup! test: regression tests for async_hooks + Promise + Worker inte…

3d9d564

…raction Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2020

devnexen approved these changes May 11, 2020

View reviewed changes

addaleax closed this May 15, 2020

addaleax deleted the async-hooks-promises-terminate branch May 15, 2020 17:38

codebytere mentioned this pull request May 18, 2020

v14.3.0 proposal #33452

Merged

codebytere mentioned this pull request Jun 9, 2020

v12.18.1 proposal #33811

Merged

codebytere added a commit to electron/electron that referenced this pull request Jun 18, 2020

Fixup async id assertion patch

40dae27

nodejs/node#33347

codebytere added a commit to electron/electron that referenced this pull request Jun 18, 2020

Fixup async id assertion patch

1d8e416

nodejs/node#33347

MylesBorins removed the lts-watch-v12.x label Nov 16, 2020

Qard mentioned this pull request Apr 21, 2021

async_hooks: use new v8::Context PromiseHook API #36394

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

async_hooks,worker: fix process.exit() crashes in async fn with async_hook enabled#33347

async_hooks,worker: fix process.exit() crashes in async fn with async_hook enabled#33347
addaleax wants to merge 4 commits intonodejs:masterfrom
addaleax:async-hooks-promises-terminate

addaleax commented May 11, 2020

Uh oh!

nodejs-github-bot commented May 11, 2020 •

edited by addaleax

Loading

Uh oh!

Uh oh!

benjamingr commented May 11, 2020

Uh oh!

addaleax commented May 11, 2020

Uh oh!

nodejs-github-bot commented May 11, 2020 •

edited by addaleax

Loading

Uh oh!

addaleax commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

addaleax commented May 11, 2020

worker: call CancelTerminateExecution() before exiting Locker

async_hooks: clear async_id_stack for terminations in more places

test: regression tests for async_hooks + Promise + Worker interaction

Checklist

Uh oh!

nodejs-github-bot commented May 11, 2020 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

benjamingr commented May 11, 2020

Uh oh!

addaleax commented May 11, 2020

Uh oh!

nodejs-github-bot commented May 11, 2020 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nodejs-github-bot commented May 11, 2020 •

edited by addaleax

Loading

nodejs-github-bot commented May 11, 2020 •

edited by addaleax

Loading