Skip to content

stream: fix early drain after Utf8Stream reopen - #65633

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-utf8stream-reopen-drain
Open

stream: fix early drain after Utf8Stream reopen#65633
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-utf8stream-reopen-drain

Conversation

@mcollina

Copy link
Copy Markdown
Member

If a 'ready' listener starts a write after reopen(), the reopen path still emits 'drain' from a nextTick before that write completes, so a listener reading the file on 'drain' can observe it empty. This is the underlying race behind the test-fastutf8stream-reopen flake that 2e8a4b1 deflaked on the test side ("The reopen path also emits a 'drain' of its own from a nextTick before the write has landed") — this fixes the library so 'drain' is never emitted while stream.writing === true.

Skip the extra emit when a write is in flight: #release() emits the real 'drain' once the write completes, so no event is lost.

The regression test defers the reopened file's fs.write by one setImmediate, which deterministically lands the write after the nextTick on which the premature 'drain' used to fire. It fails without the lib change (asserts writing === false on 'drain'; observed true, file empty) and passes with it.

Verified with 800 runs of the test under 16× parallel CPU contention: 0 failures (28/800 before the fix, reproducing the CI signature exactly).

Same fix submitted to upstream sonic-boom: pinojs/sonic-boom#255.

If a 'ready' listener starts a write after reopen(), the reopen path
still emits 'drain' from a nextTick before that write completes, so a
listener reading the file on 'drain' can observe it empty. This is the
race behind the test-fastutf8stream-reopen flake deflaked on the test
side in 2e8a4b1.

Skip the extra emit when a write is in flight: #release() emits the
real 'drain' once the write completes, so no event is lost.

The regression test defers the reopened file's fs.write by one
setImmediate, deterministically landing the write after the nextTick
on which the premature 'drain' used to fire.

Refs: nodejs@2e8a4b1a8ce
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@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. labels Aug 29, 2026
@mcollina
mcollina requested a review from jasnell August 29, 2026 05:32
@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.05%. Comparing base (7b0de5e) to head (7837e2f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65633   +/-   ##
=======================================
  Coverage   90.05%   90.05%           
=======================================
  Files         754      754           
  Lines      255722   255724    +2     
  Branches    48314    48310    -4     
=======================================
+ Hits       230281   230286    +5     
- Misses      16555    16560    +5     
+ Partials     8886     8878    -8     
Files with missing lines Coverage Δ
lib/internal/streams/fast-utf8-stream.js 80.86% <100.00%> (+0.04%) ⬆️

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

needs-ci PRs that need a full CI run. stream Issues and PRs related to Node.js streams.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants