GeezerVison

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: David Carlier <[email protected]>

nodejs/node#33347

nodejs/node#33347

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: nodejs#33347
Signed-off-by: Shelley Vohr <[email protected]>

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: nodejs#33347
Signed-off-by: Shelley Vohr <[email protected]>

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: #33347
Signed-off-by: Shelley Vohr <[email protected]>
PR-URL: #65819
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: #33347
Signed-off-by: Shelley Vohr <[email protected]>
PR-URL: #65819
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: #33347
Signed-off-by: Shelley Vohr <[email protected]>
PR-URL: #65819
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>

After `Stop(env)`, freeing the Environment and creating another one on
the same isolate failed whenever no JavaScript ran in between: the new
Environment's first script was terminated before it started. That is
the normal case when `Stop()` is called from the process exit handler
for an uncaught exception, or by an embedder while the loop is idle.
`Stop()` calls `isolate->TerminateExecution()` unless
`kDoNotTerminateIsolate` is set, and V8 only clears that request the
next time JavaScript runs, so it outlived the Environment it was meant
for.

Cancel a pending termination when the Environment it was meant for is
freed. `Worker::Run()` already did this by hand before freeing its
Environment, with a TODO asking why V8 hit a DCHECK without it; this is
why, and that call now happens in `FreeEnvironment()`.

Refs: #33347
Signed-off-by: Shelley Vohr <[email protected]>
PR-URL: #65819
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>