Skip to content

stream: preserve mutable chunks in Web Stream adapters - #64579

Open
seungwoo505 wants to merge 5 commits into
nodejs:mainfrom
seungwoo505:fix-writable-to-web-write
Open

seungwoo505 wants to merge 5 commits into
nodejs:mainfrom
seungwoo505:fix-writable-to-web-write

Conversation

@seungwoo505

@seungwoo505 seungwoo505 commented Jul 18, 2026 •

Copy link
Copy Markdown

When a Node.js writable is converted with Writable.toWeb(), the Web Streams
write() promise can currently settle as soon as the native write() call
returns true. That return value only represents backpressure, so the native
stream may still retain the supplied mutable BufferSource. Reusing the buffer
after awaiting the Web write can therefore change bytes that have not yet been
consumed.

This change:

  • waits for the native per-write callback when the stream is an uncorked,
    unmodified Writable;
  • coordinates callback completion with drain, aborts, and native stream
    errors;
  • passes a private same-brand BufferSource copy for Duplex, corked, overridden,
    and legacy write paths where waiting for a callback would change existing
    completion behavior;
  • preserves HTTP input validation before fallback copies; and
  • preserves SharedArrayBuffer backing for cloned views.

The original native Writable.prototype.write and
OutgoingMessage.prototype.write methods are captured so patched methods and
accessors are classified and invoked consistently.

Validation included:

  • make -j4 test (full test suite passed)
  • the changed Web Streams adapter, Duplex, and compression tests
  • the existing Writable, Duplex, and Web Streams adapter test set
  • CompressionStream WPT tests
  • repeated async regression tests
  • ESLint and git diff --check

Fixes: #64549

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jul 18, 2026
@seungwoo505
seungwoo505 marked this pull request as ready for review July 18, 2026 16:14
Comment thread lib/_http_outgoing.js Outdated
@@ -1260,4 +1262,5 @@ module.exports = {
validateHeaderName,
validateHeaderValue,
OutgoingMessage,
outgoingMessagePrototypeWrite,

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.

why expose this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The Web Streams adapter uses this export to compare the active write() method with the original OutgoingMessage.prototype.write.
This distinction preserves the native validation behavior without applying it to overridden write() implementations.
The reference is captured here so a later prototype monkey-patch is not mistaken for the original method.

@seungwoo505
seungwoo505 force-pushed the fix-writable-to-web-write branch from 97f5318 to c08321a Compare July 27, 2026 12:18
@seungwoo505

seungwoo505 commented Jul 27, 2026 •

Copy link
Copy Markdown
Author

Hi @bjohansebas, just a friendly follow-up on this.
I’ve answered the question above and rebased the PR onto the latest main, resolving the merge conflict.
The build, relevant Web Streams tests, and ESLint all pass locally.
When you have time, could you please take another look?
Thank you!

@bjohansebas bjohansebas added stream Issues and PRs related to Node.js streams. web streams Issues and PRs related to the Web Streams API. labels Jul 27, 2026
@mcollina

Copy link
Copy Markdown
Member

Sorry for the radio silence. Can you rebase again?

@seungwoo505

Copy link
Copy Markdown
Author

Sorry, I just saw your message.
I’ll rebase it by the end of today

Wait for native write callbacks when they can safely represent chunk
consumption. For Duplex streams, corked writes, and custom or legacy
write methods, pass private BufferSource copies to preserve completion
timing.

Coordinate callback completion with backpressure, aborts, and stream
errors so a settled Web Streams write no longer exposes mutable bytes
still retained by the native stream.

Preserve native HTTP validation and SharedArrayBuffer backing when
fallback copies are required.

Signed-off-by: seungwoo <zoozoo1302@gmail.com>
Cover native callback completion, fallback copies, aborts, and error
propagation for mutable BufferSource chunks passed to Node.js Web
Streams adapters.

Verify HTTP validation, SharedArrayBuffer backing, and Duplex and
compression paths.

Signed-off-by: seungwoo <zoozoo1302@gmail.com>
Rely on native stream validation after copying array buffer views.
Keep object-mode writes on their existing completion timing, and remove
adapter-only HTTP detection and prototype capture.

Signed-off-by: seungwoo <zoozoo1302@gmail.com>
Remove overlapping override coverage. Verify object-mode writes settle
independently of native callbacks. Exercise the Writable.toWeb(Duplex)
path and keep native HTTP validation coverage.

Assisted-by: Codex
Signed-off-by: seungwoo <zoozoo1302@gmail.com>
@seungwoo505
seungwoo505 force-pushed the fix-writable-to-web-write branch from c08321a to 062b9a0 Compare September 28, 2026 10:50
@seungwoo505

seungwoo505 commented Sep 28, 2026 •

Copy link
Copy Markdown
Author

Hi @mcollina, I’ve rebased the PR onto the latest main and resolved the conflicts.
git diff --check, make -j4, make test, the relevant Web Streams tests, and the targeted JavaScript lint checks all pass locally.
Could you please take a look when you have a chance?
Thanks!

Use the Float16Array constructor captured during Node.js initialization
instead of reading it from globalThis when the adapter is loaded.
This prevents changes to the global constructor from affecting chunk
cloning.

Signed-off-by: seungwoo <zoozoo1302@gmail.com>

@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. Only starts once the PR has an approving review. label Sep 29, 2026
@mcollina
mcollina requested a review from panva September 29, 2026 10:22
@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Benchmark GHA (webstreams / adapters): https://github.com/nodejs/node/actions/runs/36556500893

Results

[!WARNING]
Do not take GHA benchmark results as face value, always confirm them
using a dedicated machine, e.g. Jenkins CI.

Benchmark results:

                                                         confidence improvement accuracy (*)    (**)   (***)
webstreams/adapters.js kind='readable-from-web' n=100000                 1.94 %       ±8.32% ±10.97% ±14.07%
webstreams/adapters.js kind='readable-to-web' n=100000                   1.36 %       ±8.69% ±11.46% ±14.70%
webstreams/adapters.js kind='writable-from-web' n=100000                -0.54 %       ±7.68% ±10.13% ±13.00%
webstreams/adapters.js kind='writable-to-web' n=100000          ***    -81.09 %       ±6.21%  ±8.21% ±10.57%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

[!WARNING]
Do not take GHA benchmark results as face value, always confirm them
using a dedicated machine, e.g. Jenkins CI.

@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 Sep 29, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva

panva commented Sep 29, 2026

Copy link
Copy Markdown
Member

The benchmark shows a significant drop for writable-to-web. I think this needs some work before landing.

@seungwoo505

Copy link
Copy Markdown
Author

I’ve confirmed the performance regression shown in the benchmark.
I’ll work on addressing it as soon as possible.

@codecov

codecov Bot commented Sep 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.71207% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.36%. Comparing base (75e4bbe) to head (3c2a3c3).
⚠️ Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/webstreams/adapters.js 90.35% 29 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64579      +/-   ##
==========================================
- Coverage   90.36%   90.36%   -0.01%     
==========================================
  Files         792      792              
  Lines      275398   275793     +395     
  Branches    52776    52852      +76     
==========================================
+ Hits       248877   249210     +333     
- Misses      16937    17005      +68     
+ Partials     9584     9578       -6     
Files with missing lines Coverage Δ
lib/internal/streams/writable.js 96.44% <100.00%> (+0.01%) ⬆️
lib/internal/webstreams/writablestream.js 99.52% <100.00%> (+<0.01%) ⬆️
lib/internal/webstreams/adapters.js 88.53% <90.35%> (+0.29%) ⬆️

... and 40 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.

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

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

stream: Writable.toWeb()/Duplex.toWeb() settles write() before a mutable chunk is consumed

5 participants