Repository navigation
fs.Utf8Stream: don't emit post-reopen 'drain' while a write is in flight #36412
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, tempDir } from "harness"; | ||
|
|
||
| // Deflake for test/js/node/test/parallel/test-fastutf8stream-reopen.js. | ||
| // | ||
| // After reopen(), fileOpened() emits 'ready' synchronously (async mode). If a | ||
| // 'ready' listener calls write(), #actualWrite starts an async fs.write and | ||
| // sets #writing=true. fileOpened() must then NOT schedule its post-reopen | ||
| // nextTick 'drain': that drain would fire while the write is still in flight, | ||
| // so a listener that reads the file on 'drain' observes an empty file. The | ||
| // real flake surfaces when Bun's fs.write threadpool task completes after | ||
| // nextTick under CI load; here we force it deterministically by deferring the | ||
| // write callback with setImmediate. | ||
| test("fs.Utf8Stream: reopen does not emit 'drain' while a write started from 'ready' is pending", async () => { | ||
| using dir = tempDir("utf8stream-reopen-drain", {}); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const { Utf8Stream } = require("node:fs"); | ||
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
| const assert = require("node:assert"); | ||
|
|
||
| const dest = path.join(process.env.DIR, "a.log"); | ||
| let afterReopen = false; | ||
| const slowFs = { | ||
| write(fd, data, enc, cb) { | ||
| if (afterReopen) { | ||
| setImmediate(() => fs.write(fd, data, enc, cb)); | ||
| } else { | ||
| fs.write(fd, data, enc, cb); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const stream = new Utf8Stream({ dest, sync: false, fs: slowFs }); | ||
| assert.ok(stream.write("hello world\\n")); | ||
| assert.ok(stream.write("something else\\n")); | ||
| const after = dest + "-moved"; | ||
|
|
||
| stream.once("drain", () => { | ||
| fs.renameSync(dest, after); | ||
| stream.reopen(); | ||
| stream.once("ready", () => { | ||
| afterReopen = true; | ||
| assert.ok(stream.write("after reopen\\n")); | ||
| stream.once("drain", () => { | ||
| assert.strictEqual(stream.writing, false, "'drain' fired while a write is in flight"); | ||
| assert.strictEqual(fs.readFileSync(after, "utf8"), "hello world\\nsomething else\\n"); | ||
| assert.strictEqual(fs.readFileSync(dest, "utf8"), "after reopen\\n"); | ||
| stream.end(); | ||
| stream.on("close", () => console.log("PASS")); | ||
| }); | ||
| }); | ||
| }); | ||
| `, | ||
| ], | ||
| env: { ...bunEnv, DIR: String(dir) }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stderr).toBe(""); | ||
| expect(stdout).toBe("PASS\n"); | ||
| expect(exitCode).toBe(0); | ||
| }); |
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 sibling
|| this.#flushPendingclause on this same conditional has the identical hazard and remains unguarded — if a flush is pending across a reopen and a'ready'listener starts an async write,fileOpenedcalls#actualWrite()a second time on the in-flight#writingBuf, issuing two concurrentfs.writecalls (duplicate bytes + double#release). Hoisting!this.#writing &&around the whole disjunction fixes both arms. This is pre-existing (matches upstream Node) and needs a narrower trigger (minLength > 0+flush(cb)pending across a reopen), so it's a nit, not a blocker.Extended reasoning...
What the bug is
The PR correctly identifies that code in
fileOpenedrunning after the synchronousemit('ready')cannot assume#writingis still false, because a'ready'listener may have calledwrite()and started an asyncfs.write. It guards theelse if (reopening)branch accordingly. However, the immediately preceding branch on the same line has the identical hazard on its|| this.#flushPendingclause, and that clause is not gated on!this.#writing:If
#flushPendingis true whenfileOpenedruns and a'ready'listener has just started an async write (#writing = true), the first branch is taken via|| this.#flushPendingand#actualWrite()is called again.#actualWriteUtf8doesthis.#writingBuf ||= this.#bufs.shift() || ""— since#writingBufis already the non-empty in-flight payload, the||=keeps it and#fsWrite()issues a second concurrentfs.write(fd, sameBuf). Both completions invoke the sharedboundRelease.Step-by-step proof (async mode,
minLength > 0)new Utf8Stream({ dest, sync: false, minLength: 8 });write("abc")buffers (3 < 8, no write).flush(cb)→#flushUtf8:minLength > 0, so#callFlushCallbackOnDrain(cb)sets#flushPending = trueand registersonce('drain', onDrain); then#actualWrite()starts an asyncfs.write(#writing = true).reopen()→#reopening = true; returns early because#writing.fs.writefrom step 2 completes →#releasesees#reopening, takes the reopen branch without emitting'drain'(soonDrainnever fires and#flushPendingstaystrue), and callsreopen()→#openFile.fs.opencompletes →fileOpened: clears#reopening/#opening/#writing, then synchronouslyemit('ready').once('ready')listener callswrite("after reopen\n")(13 ≥ 8) →#writeUtf8hits!this.#writing && this.#len >= this.#minLength→#actualWrite()→#writing = true,#writingBuf = "after reopen\n", asyncfs.writein flight.fileOpened.(!this.#writing && …)is false, but|| this.#flushPendingis true →#actualWrite()runs again →#writingBuf ||= …keeps"after reopen\n"→ second#fsWrite()on the same fd with the same payload.Why existing code doesn't prevent it
#flushPendingis set only in#callFlushCallbackOnDrainand cleared only by its'drain'/'error'listeners.#release's#reopeningbranch does not emit'drain', and#openFileclears#asyncDrainScheduledbut not#flushPending, so the flag survives intofileOpened.#actualWriteUtf8's||=is designed for the partial-write retry path; it has no way to know a write is already in flight, so it happily re-issues.Impact
Two concurrent
fs.writecalls on the same fd in append mode → the payload is written to disk twice. Both completions callboundRelease, so#releaseruns twice: the first drains#writingBufand (with#lennow 0) emits'drain'; the second runs against already-cleared state, likely emitting a spurious extra'drain'and desynchronizing#lenbookkeeping.Fix
Same fix as the PR's, applied to the sibling clause — hoist the guard around the whole disjunction:
When
#writingis already true,#releaseon the in-flight write will pick up the pending flush via its normallen > this.#minLength/ drain path.Severity
Nit. This is pre-existing (predates the PR and matches upstream Node's
fast-utf8-stream.jsidentically), and the trigger is materially narrower than the bug the PR fixes: it needsminLength > 0, aflush(cb)pending across a reopen, and a'ready'listener that writes ≥minLengthbytes. The observed CI flake is fully addressed by the PR as-is. Flagging per REVIEW.md's "fix the whole class in the same PR — parallel switch arms" since it's the same hazard on the same conditional the PR edits, and the one-token hoist covers both — but it shouldn't block merging this deflake.