Skip to content

stream: fix pipeToSync byte accounting#63564

Draft
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:stream-iter-pipetosync-writesync-backpressure
Draft

stream: fix pipeToSync byte accounting#63564
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:stream-iter-pipetosync-writesync-backpressure

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented May 25, 2026

Fixes: #63562

pipeToSync() ignored explicit false returns from writeSync() and
writevSync(), so it could report bytes for chunks that were not accepted by
the writer.

This updates pipeToSync() to stop counting rejected sync writes while still
preserving writers that intentionally return false after accepting data as a
backpressure signal.


Assisted-by: openai:gpt-5.5

@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 the stream subsystem. labels May 25, 2026
Stop pipeToSync() from counting chunks when writeSync() or writevSync()
returns false without accepting the data. Preserve writers that use
false as an accepted backpressure signal.

Fixes: nodejs#63562

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: openai:gpt-5.5
@trivikr trivikr force-pushed the stream-iter-pipetosync-writesync-backpressure branch from 0b933ff to 8b50431 Compare May 25, 2026 16:54
@trivikr trivikr changed the title stream/iter: fix pipeToSync byte accounting stream: fix pipeToSync byte accounting May 25, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 25, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (49dcaa0) to head (8b50431).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/streams/iter/pull.js 88.88% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #63564    +/-   ##
========================================
  Coverage   90.33%   90.33%            
========================================
  Files         730      730            
  Lines      234362   234487   +125     
  Branches    43906    43932    +26     
========================================
+ Hits       211713   211835   +122     
- Misses      14371    14389    +18     
+ Partials     8278     8263    -15     
Files with missing lines Coverage Δ
lib/internal/streams/iter/pull.js 88.09% <88.88%> (-0.03%) ⬇️

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

@trivikr trivikr marked this pull request as draft May 25, 2026 23:01
@trivikr
Copy link
Copy Markdown
Member Author

trivikr commented May 25, 2026

Converted to draft as behavior should be confirmed in specification

Details #63562 (comment)

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

Labels

needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stream/iter: pipeToSync() ignores writeSync() backpressure and overcounts bytes

2 participants