Skip to content

stream: fix pipeToSync byte accounting#63564

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

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
@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 125 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

This comment was marked as outdated.

@trivikr trivikr marked this pull request as ready for review June 8, 2026 01:49
@trivikr trivikr force-pushed the stream-iter-pipetosync-writesync-backpressure branch from 8b50431 to 7daf002 Compare June 8, 2026 01:49
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 7daf002 to 70cf4f8 Compare June 8, 2026 02:06
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