-
Notifications
You must be signed in to change notification settings - Fork 5k
FileSink: resolve the pending write()'s promise when end()'s flush drains it #35397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+122
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The
readerIIFE is only awaited inside the try body (line 345), not infinally— ifexpect(await writePromise).toBe(chunkSize)at line 344 throws,finallyclosesreadFd, and on the next tickdrainSync()hits EBADF → caught →breakinner loop withdrainedunchanged → outerwhile (drained < total)re-awaitssetImmediateforever, leaking a spinning loop into later tests. The sibling tests in this file putawait readerinfinallyfor exactly this reason; add adoneflag the loop checks, set it infinallybefore closing the fds, andawait readerthere.Extended reasoning...
What the bug is
The new test creates a background reader IIFE at lines 336-342 that loops
while (drained < total), callingdrainSync()and thenawait new Promise(r => setImmediate(r))each iteration. The only place this reader is awaited is line 345, inside the try body — thefinallyblock closeswriteFdandreadFdbut never signals or awaitsreader.The specific code path that triggers it
If line 344 —
expect(await writePromise).toBe(chunkSize)— throws (either becausewritePromiserejects, or because it resolves to something other thanchunkSize, i.e. exactly the regression this test guards against), control jumps straight tofinallywithout ever reachingawait reader. Thefinallyblock then doesfs.closeSync(readFd).Step-by-step proof
finally.finallyrunsfs.closeSync(readFd). The reader IIFE is still suspended onsetImmediate.drainSync()→fs.readSync(readFd, buf)on the closed fd → throwsEBADF.catch { break }breaks only the innerfor(;;)loop.drainedwas not incremented.drained < totalis still true (nothing changed), so it does not exit.await new Promise(r => setImmediate(r))→ step 3 repeats forever.The
setImmediatehandle keeps the event loop referenced each tick, so this leaks a hot-spinning loop into every subsequent test in the file. Worse, closed fd numbers are recycled — a later test that opens a socket may get the same fd number, and this loop will silentlyreadSyncfrom that unrelated fd.Why existing code doesn't prevent it
drainSync()'s catch handler was written for the happy-path EAGAIN case (nonblocking socket has no more data right now), where breaking the inner loop and re-polling viasetImmediateis correct. It doesn't distinguish EBADF, and nothing else terminates the outerwhile. Thefinallyblock has no reference toreaderat all.Impact
This only bites when the test is already failing — the
endResult === writePromiseassertion at line 333 (the primary regression guard) fires before the reader is created, so the leak window is exactly line 344. But line 344 is the test's core resolution-value assertion; a future regression inpending.consumedaccounting would make it fail and then poison the rest of the suite on persistent CI runners. This is precisely what REVIEW.md's rule targets: "Release every resource via using/await using or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners)".How to fix
Match the sibling tests in this file (e.g. "a backpressured write() resolves to the chunk's byte count"), which put
await readerinsidefinally. Since this reader has no natural EOF, add a termination flag:and in
finally, before closing the fds:(or equivalently, bound the loop with an iteration cap).