GeezerVison

Copy link

Copy Markdown

Member

bricss, KhafraDev, paulrutter, Eomm, and tonivj5 reacted with thumbs up emoji jaydenseric and tonivj5 reacted with eyes emoji

legendecas force-pushed the platform-tasks branch from f9c4eb9 to 0cd1a72 Compare December 26, 2023 17:51

legendecas force-pushed the platform-tasks branch from 0cd1a72 to 8324ab0 Compare December 26, 2023 17:53

bcoe reviewed Dec 27, 2023

View reviewed changes

legendecas force-pushed the platform-tasks branch from 8324ab0 to beab806 Compare December 28, 2023 14:59

bcoe reviewed Dec 28, 2023

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

H4ad approved these changes Dec 28, 2023

View reviewed changes

mcollina approved these changes Jan 8, 2024

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas deleted the platform-tasks branch January 8, 2024 14:41

At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: #51290
Fixes: #47748
Fixes: #49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: nodejs#51290
Fixes: nodejs#47748
Fixes: nodejs#49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: nodejs#51290
Fixes: nodejs#47748
Fixes: nodejs#49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: #51290
Fixes: #47748
Fixes: #49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

nodejs/node#51290

nodejs/node#51290

* chore: bump node in DEPS to v20.12.0

* chore: update build_add_gn_build_files.patch

* chore: update patches

* chore: bump node in DEPS to v20.12.1

* chore: update patches

* build: encode non-ASCII Latin1 characters as one byte in JS2C

nodejs/node#51605

* crypto: use EVP_MD_fetch and cache EVP_MD for hashes

nodejs/node#51034

* chore: update filenames.json

* chore: bump node in DEPS to v20.12.2

* chore: update patches

* src: support configurable snapshot

nodejs/node#50453

* test: remove test-domain-error-types flaky designation

nodejs/node#51717

* src: avoid draining platform tasks at FreeEnvironment

nodejs/node#51290

* chore: fix accidentally deleted v8 dep

* lib: define FormData and fetch etc. in the built-in snapshot

nodejs/node#51598

* chore: rebase on main

* chore: remove stray log

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Cheng <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

At the point of `FreeEnvironment` and onwards, no JavaScript execution
associated with the Environment should be triggered.

Avoid draining platform tasks that can trigger JavaScript execution in
`FreeEnvironment`. The holder of `node::Environment` should immediately
call `node::MultiIsolatePlatform::UnregisterIsolate` and
`v8::Isolate::Dispose` to cancel pending foreground tasks and join
concurrent tasks after the environment was freed.

`NodePlatform` can properly handle the case in `RunForegroundTask` when
an Isolate out-lives its associated `node::Environment`.

PR-URL: #51290
Fixes: #47748
Fixes: #49344
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

nodejs/node#51290

* chore: bump node in DEPS to v20.13.1

* chore: bump node in DEPS to v20.14.0

* chore: update build_add_gn_build_files.patch

* chore: update patches

* chore: update patches

* build: encode non-ASCII Latin1 characters as one byte in JS2C

nodejs/node#51605

* crypto: use EVP_MD_fetch and cache EVP_MD for hashes

nodejs/node#51034

* chore: update filenames.json

* chore: update patches

* src: support configurable snapshot

nodejs/node#50453

* test: remove test-domain-error-types flaky designation

nodejs/node#51717

* src: avoid draining platform tasks at FreeEnvironment

nodejs/node#51290

* chore: fix accidentally deleted v8 dep

* lib: define FormData and fetch etc. in the built-in snapshot

nodejs/node#51598

* chore: remove stray log

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: fixup

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: fixup indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Cheng <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

…velock (#218)

## Problem

The recurring `frontend_unit_tests` TIMEOUT flake (`close timed out
after 120000ms` → `Failed to terminate worker while running ...`) is
caused by a bug in Node 18.19.1 itself, not by test code.

gdb backtrace of a live hung worker shows the livelock: on the worker's
**normal exit path**, `FreeEnvironment() → NodePlatform::DrainTasks()`
tries to drain the foreground task queue, but
`FinalizationRegistryCleanupTask`s keep re-spawning faster than they
drain — a native infinite loop at 100% CPU that `worker.terminate()`
cannot interrupt. The FinalizationRegistry instances come from jsdom's
`iterable-weak-set` (one per Document), which is why every test file is
equally exposed and the blamed filename in the error is arbitrary.

Upstream fix: nodejs/node#51290 (fixes nodejs/node#47748 "Infinite loop
at shutdown"), backported to Node 18.20.3+. We are pinned one patch line
short at 18.19.1.

## Fix

Bump the bazel Node toolchain to 18.20.4 — the newest 18.x bundled with
rules_nodejs 6.3.0, so no rules bump is needed.

## Verification

A/B/C stress runs of shard 10/10 under identical 9-shard concurrent load
(mimicking bazel's parallel shard execution):

| Node | Hangs |
|---|---|
| 18.19.1 (current) | 12 / 31 iterations (~39%) |
| 18.20.4 (this PR) | 0 / 60 |
| 18.20.8 | 0 / 60 |

Full bazel frontend suite on the 18.20.4 toolchain: 10/10 shards pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <[email protected]>

…velock (#218)

## Problem

The recurring `frontend_unit_tests` TIMEOUT flake (`close timed out
after 120000ms` → `Failed to terminate worker while running ...`) is
caused by a bug in Node 18.19.1 itself, not by test code.

gdb backtrace of a live hung worker shows the livelock: on the worker's
**normal exit path**, `FreeEnvironment() → NodePlatform::DrainTasks()`
tries to drain the foreground task queue, but
`FinalizationRegistryCleanupTask`s keep re-spawning faster than they
drain — a native infinite loop at 100% CPU that `worker.terminate()`
cannot interrupt. The FinalizationRegistry instances come from jsdom's
`iterable-weak-set` (one per Document), which is why every test file is
equally exposed and the blamed filename in the error is arbitrary.

Upstream fix: nodejs/node#51290 (fixes nodejs/node#47748 "Infinite loop
at shutdown"), backported to Node 18.20.3+. We are pinned one patch line
short at 18.19.1.

## Fix

Bump the bazel Node toolchain to 18.20.4 — the newest 18.x bundled with
rules_nodejs 6.3.0, so no rules bump is needed.

## Verification

A/B/C stress runs of shard 10/10 under identical 9-shard concurrent load
(mimicking bazel's parallel shard execution):

| Node | Hangs |
|---|---|
| 18.19.1 (current) | 12 / 31 iterations (~39%) |
| 18.20.4 (this PR) | 0 / 60 |
| 18.20.8 | 0 / 60 |

Full bazel frontend suite on the 18.20.4 toolchain: 10/10 shards pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5 <[email protected]>
(cherry picked from commit d93131d)

An async WebAssembly.compile() job still running on the platform worker
threads when a program was torn down could crash the process in
uv_async_send() (SIGSEGV at 0x78, a NULL uv_async_t): the
node::CommonEnvironmentSetup destructor unregisters the isolate from the
platform before disposing of it, and a compilation step completing in
between posts a foreground task to the per-isolate task runner whose
async handle is concurrently being nulled and closed.  Node's
FreeEnvironment() no longer drains the platform (nodejs/node#51290), so
deleteIntern() now drains all platform tasks after node::Stop(), while
the isolate is still registered, with JavaScript execution disallowed.

Also fixes bugs found while testing:
- JavaScriptPromise: the promise callback info held a V8 persistent
  handle without a weak program reference, so releasing a promise after
  its program was destroyed reset the handle in a disposed isolate
  (use-after-free)
- node's per-process initialization marked the process' stdio file
  descriptors close-on-exec, so all child processes started after the
  module was loaded (system(), backquote(), ...) ran without stdin,
  stdout, and stderr; stdio inheritance is now enabled
- JavaScriptObject::toData(): member functions of objects were bound to
  the function itself instead of the containing object as 'this'

Node before 20.12.0 can spin at 100% CPU forever while a process exits
(nodejs/node#47748, fixed by nodejs/node#51290 in 20.12.0). It hung the
CI test job on Node 20.1.0, and a pair command, which talks to the hub
with fetch and exits on its own, can hit it too. Node 20 also reached
its end of life on April 30, 2026, so pair now requires the oldest Node
line that Node still supports.

engines.node is >=22.0.0, so requireNode() and pair check name 22.0.0.
CI tests 22.0.0, 22 and 24 in place of 20.1.0, 20 and 24, and the
README, the docs and guide/setup.md say Node 22.0.0 or newer.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>