Skip to content

stream: fix uncatchable error closing a half-open Duplex.toWeb()#64161

Open
3zrv wants to merge 2 commits into
nodejs:mainfrom
3zrv:fix/half-open-duplex-error-catch
Open

stream: fix uncatchable error closing a half-open Duplex.toWeb()#64161
3zrv wants to merge 2 commits into
nodejs:mainfrom
3zrv:fix/half-open-duplex-error-catch

Conversation

@3zrv

@3zrv 3zrv commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Fixes: #64120

What: Fix an uncatchable TypeError: Cannot read properties of undefined (reading 'error') thrown from internal/webstreams/adapters when closing the writable side of a half-open Duplex.toWeb() (e.g. a net socket whose peer sent FIN).

Why: The writable adapter's close() has an else-branch (taken when writableEnded is already true) that sets controller = undefined but does not detach the eos listener. When the underlying writable's finish/close fires afterward, the eos callback runs controller.error(new AbortError()) on the now-undefined controller and throws. Because the throw happens synchronously inside the finish event emit (not in the close() promise chain), user try/catch around writer.close() cannot catch it and the process crashes.

This was latent until v26.3.0. Commit cbee0de (align Readable.toWeb termination with eos) made the readable side's eos use writable: false, so reader.read() now resolves on the readable END before the writable FINISH. With allowHalfOpen: false, that lands writer.close() in the exact window where writableEnded === true but writableFinished === false, triggering the null-deref.

How: Guard both controller.error(...) calls in the eos callback with optional chaining (controller?.error(...)), completing the intent that the controller is inert once close() clears it. The eos listener is intentionally kept attached so its error-handling safety net still swallows any post-close error rather than letting that become uncaught. Adds a deterministic, network-free regression test (a Duplex with allowHalfOpen: false and a final() that defers finish to a macrotask).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jun 27, 2026
@3zrv 3zrv changed the title stream: fix uncatchable error closing a half-open Duplex.toWeb() writ… stream: fix uncatchable error closing a half-open Duplex.toWeb() Jun 27, 2026

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina

Copy link
Copy Markdown
Member

Can you add the DCO signoff?

@3zrv

3zrv commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

I think my commit already has it

Signed-off-by: Mohamed Sayed k@3zrv.com

Unless there's something I missed

@jazelly jazelly added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 29, 2026
@jazelly

jazelly commented Jun 29, 2026

Copy link
Copy Markdown
Member

lgtm! Can you please amend your first commit message? The title needs to be less than 72 chars.

@3zrv 3zrv force-pushed the fix/half-open-duplex-error-catch branch from fa05a8e to e16d5b1 Compare June 29, 2026 13:27

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 29, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina

mcollina commented Jul 1, 2026

Copy link
Copy Markdown
Member

@nodejs/platform-windows @nodejs/build can you take a look at why this test is now failing on Windows (https://ci.nodejs.org/job/node-test-binary-windows-js-suites/41387/RUN_SUBSET=1,nodes=win10-COMPILED_BY-vs2022_clang/testReport/junit/(root)/parallel/test_path_win32_normalize_device_names/):

---
duration_ms: 222.997
exitcode: 1
severity: fail
stack: |-
  node:internal/assert/utils:146
    throw error;
    ^

  AssertionError [ERR_ASSERTION]: path.win32.normalize("COM9.") === ".\\COM9.", but got "COM9."
  + actual - expected

  + 'COM9.'
  - '.\\COM9.'

      at Object.<anonymous> (c:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-path-win32-normalize-device-names.js:114:10)
      at Module._compile (node:internal/modules/cjs/loader:1947:14)
      at Object..js (node:internal/modules/cjs/loader:2087:10)
      at Module.load (node:internal/modules/cjs/loader:1669:32)
      at Module._load (node:internal/modules/cjs/loader:1450:12)
      at wrapModuleLoad (node:internal/modules/cjs/loader:260:19)
      at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:154:5)
      at node:internal/main/run_main_module:33:47 {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: 'COM9.',
    expected: '.\\COM9.',
    operator: 'strictEqual',
    diff: 'simple'
  }

  Node.js v27.0.0-pre
...

This PR is absolutely not related.

@targos

targos commented Jul 1, 2026

Copy link
Copy Markdown
Member

You need c4429c8 (rerunning a full CI should rebase and include it)

@3zrv

3zrv commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

should be good now

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 1, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jazelly jazelly added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jul 8, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 8, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/64161
βœ”  Done loading data for nodejs/node/pull/64161
----------------------------------- PR info ------------------------------------
Title      stream: fix uncatchable error closing a half-open Duplex.toWeb() (#64161)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     3zrv:fix/half-open-duplex-error-catch -> nodejs:main
Labels     author ready, web streams
Commits    2
 - stream: fix uncatchable error closing half-open Duplex.toWeb() writable
 - Merge branch 'main' into fix/half-open-duplex-error-catch
Committers 1
 - Mohamed Sayed <k@3zrv.com>
PR-URL: https://github.com/nodejs/node/pull/64161
Fixes: https://github.com/nodejs/node/issues/64120
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64161
Fixes: https://github.com/nodejs/node/issues/64120
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
--------------------------------------------------------------------------------
   β„Ή  This PR was created on Sat, 27 Jun 2026 02:53:50 GMT
   βœ”  Approvals: 2
   βœ”  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/64161#pullrequestreview-4609329978
   βœ”  - Jason Zhang (@jazelly): https://github.com/nodejs/node/pull/64161#pullrequestreview-4591853232
   ⚠  GitHub cannot link the author of 'stream: fix uncatchable error closing half-open Duplex.toWeb() writable' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   ⚠  GitHub cannot link the author of 'Merge branch 'main' into fix/half-open-duplex-error-catch' to their GitHub account.
   ⚠  Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork
   βœ”  Last GitHub CI successful
   β„Ή  Last Full PR CI on 2026-07-08T12:26:38Z: https://ci.nodejs.org/job/node-test-pull-request/74658/
- Querying data for job/node-test-pull-request/74658/
βœ”  Build data downloaded
   βœ”  Last Jenkins CI successful
--------------------------------------------------------------------------------
   βœ”  No git cherry-pick in progress
   βœ”  No git am in progress
   βœ”  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
βœ”  origin/main is now up-to-date
- Downloading patch for 64161
From https://github.com/nodejs/node
 * branch                  refs/pull/64161/merge -> FETCH_HEAD
βœ”  Fetched commits as 9051e5e7eb3f..89be1dc513bc
--------------------------------------------------------------------------------
error: commit 89be1dc513bc4fe548301d5e6c48920723c35755 is a merge but no -m option was given.
fatal: cherry-pick failed
[main d46f5d2e64] stream: fix uncatchable error closing half-open Duplex.toWeb() writable
 Author: Mohamed Sayed <k@3zrv.com>
 Date: Sat Jun 27 05:44:35 2026 +0300
 2 files changed, 27 insertions(+), 2 deletions(-)
   ✘  Failed to apply patches
https://github.com/nodejs/node/actions/runs/28949193825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Uncatchable exception from webstream adapter

5 participants