Skip to content

stream: align Readable.toWeb termination with eos - #62394

Merged
aduh95 merged 12 commits into
nodejs:mainfrom
ikeyan:fix-stream-readable-to-web
May 19, 2026
Merged

aduh95 merged 12 commits into
nodejs:mainfrom
ikeyan:fix-stream-readable-to-web

Conversation

@ikeyan

@ikeyan ikeyan commented Mar 22, 2026

Copy link
Copy Markdown
Contributor

This aligns Readable.toWeb(stream) termination handling with
eos(stream, { writable: false }).

Changes:

  • add regression tests for half-open duplex termination
  • add regression coverage for adapter creation before and after
    end(), destroy(), and destroy(error)
  • preserve terminated Readable adapter state, including closed/error
    state and byte stream type, regardless of when the adapter is created
  • let Readable.toWeb() reuse eos() immediate completion handling
    for already-terminated streams
  • add regression coverage for the synchronous internal EOS callback
    path, including listener cleanup and BYOB termination

Tests:

  • make lint
  • ./configure && make -j4 test

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams. web streams Issues and PRs related to the Web Streams API. labels Mar 22, 2026
@codecov

codecov Bot commented Mar 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.69%. Comparing base (8199f9c) to head (4922bc4).
⚠️ Report is 529 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62394      +/-   ##
==========================================
- Coverage   89.69%   89.69%   -0.01%     
==========================================
  Files         676      676              
  Lines      206693   206767      +74     
  Branches    39577    39588      +11     
==========================================
+ Hits       185402   185457      +55     
- Misses      13435    13442       +7     
- Partials     7856     7868      +12     
Files with missing lines Coverage Δ
lib/internal/streams/end-of-stream.js 98.52% <100.00%> (+1.10%) ⬆️
lib/internal/webstreams/adapters.js 86.43% <100.00%> (-0.02%) ⬇️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MattiasBuelens MattiasBuelens added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 23, 2026
@ikeyan
ikeyan force-pushed the fix-stream-readable-to-web branch from b9b8a96 to a0fd4b7 Compare March 24, 2026 02:11
@ikeyan

ikeyan commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Nothing to do now. Ready to merge.

@MattiasBuelens MattiasBuelens added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Mar 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels May 19, 2026
@aduh95
aduh95 merged commit 783b382 into nodejs:main May 19, 2026
74 checks passed
@aduh95

aduh95 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Landed in 783b382

aduh95 pushed a commit that referenced this pull request May 22, 2026
PR-URL: #62394
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ikeyan
ikeyan deleted the fix-stream-readable-to-web branch June 29, 2026 06:15
aduh95 pushed a commit to aduh95/node that referenced this pull request Jul 30, 2026
PR-URL: nodejs#62394
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
PR-URL: #62394
Backport-PR-URL: #64675
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell added a commit to cloudflare/workerd that referenced this pull request Sep 14, 2026
newReadableStreamFromStreamReadable watched its source with a plain
eos(stream, cb), which for a Duplex settles only once both sides are
done. The readable half of Duplex.toWeb() (and Readable.toWeb() over any
Duplex) therefore never reported done for a source whose readable side
had ended while its writable side stayed open: a read() issued after the
EOF hung until the writer was closed, which for a half-open socket whose
peer has sent FIN may never happen. Watch with { writable: false }, as
upstream does since v26.3.0 (nodejs/node#62394), so the ReadableStream
mirrors the readable side only. The writable half keeps watching the
whole Duplex, so writer.close() still settles after the readable side
has ended, matching upstream's Writable.toWeb.

The suite's pin of the old coupling is replaced by one asserting that
the EOF read resolves while the writable side is open and that the
writable half keeps working afterwards.

Compatibility: no flag. The only observable change is that a read()
which previously stayed pending until the writable side finished now
resolves with done; no read that previously settled changes its result,
and non-Duplex sources are unaffected since the option is a no-op for a
stream with no writable side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams. web streams Issues and PRs related to the Web Streams API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants