-
Notifications
You must be signed in to change notification settings - Fork 5k
fs: keep WriteStream short-write retries at the current offset #36136
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
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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.
🟡 Same-class site not covered:
FdVectorIo::from_js(node_fs.rs:2829-2830, backingfs.writev/writevSync/readv/readvSync) still doesSome(pos_value.to_int64() as u64)whenis_number(), sofs.writev(fd, bufs, NaN, cb)still becomespwritevat offset 0 instead of the current file offset. Node'sWriteBuffers/ReadBuffersroute position through the sameGetOffsethelper this PR mirrors, sowrite_position_from_js(adapted forOption<u64>) should replace theto_int64()coercion there too. The WriteStream corruption path is closed by the streams.ts fix (writevAll no longer passes NaN), so this only affects direct user calls with non-safe-integer positions — mentioning per REVIEW.md "fix the whole class in the same PR".Extended reasoning...
What
The PR introduces
write_position_from_js()to mirror Node'sGetOffsetsemantics (only a non-negative safe integer selects positional I/O; NaN/±Infinity/non-integers/negatives fall back to the current file offset) and applies it to thefs.write/fs.writeSyncargument parser at both call sites. However, the sibling parserFdVectorIo::from_jsatnode_fs.rs:2827-2830— aliased as bothargs::Writevandargs::Readv(:2848-2849) and therefore backingfs.writev,fs.writevSync,fs.readv, andfs.readvSync— still does:NaNpassesis_number(), andJSValue::to_int64(NaN)returns0(as the PR description itself confirms for thefs.writecase). Sofs.writev(fd, bufs, NaN, cb)still issuespwritevat offset 0 rather thanwritevat the current file offset.±Infinityand non-integers like1.5similarly get coerced to a positional offset rather than falling through to-1.Why the JS layer doesn't intercept it
The
fs.writevwrapper insrc/js/node/fs.tspassespositionstraight through to the native binding with no coercion, so a user-suppliedNaNreachesFdVectorIo::from_jsunmodified.Node parity
Node's
binding.writeBuffersandbinding.readBuffers(src/node_file.cc) both routeargs[2]throughGetOffset, which is exactly the helperwrite_position_from_jswas written to emulate:IsSafeJsInt(v) ? v->IntegerValue() : -1. So Node'sfs.writev(fd, bufs, NaN, cb)writes at the current file offset, while Bun after this PR still writes at offset 0.Step-by-step
const fd = fs.openSync(p, 'r+')on a 10-byte file ofAAAAAAAAAA.fs.readSync(fd, Buffer.alloc(5), 0, 5, null)— advances the fd cursor to offset 5.fs.writevSync(fd, [Buffer.from('XX')], NaN).NaNthrough unchanged →FdVectorIo::from_js.NaN.is_number()is true →position = Some(to_int64(NaN) as u64) = Some(0).pwritev_innerseesSome(0)→ issuespwritev(fd, iov, 0).XXAAAAAAAA. Node writes at the cursor and producesAAAAAXXAAA.The new
writeSync > treats position %s as the current file offsettest in this PR would fail if extended to coverfs.writevSync/fs.readvSync.Impact / severity
This is nit severity. The primary bug this PR set out to fix — WriteStream retries stamping the tail over the file head — is fully closed:
writevAllnow passes the capturedpos(which staysundefinedon retry when nostartwas given), soNaNnever reachesfs.writevfrom the WriteStream path anymore. The remaining divergence is only reachable via direct user calls tofs.writev/fs.readvwith a pathological non-safe-integer position, which is pre-existing behavior and not a regression. Flagging it because REVIEW.md asks to "fix the whole class in the same PR — grep for every sibling site sharing the pattern", and this is the exact sibling of the parser the PR just fixed.Fix
Replace the
is_number()→to_int64()branch inFdVectorIo::from_jswith the same safe-integer gate used inwrite_position_from_js(adapted forOption<u64>, since this parser storesu64rather thani64), and extend the new position-coercion test to coverwritevSync/readvSync.