Conversation
A file-backed Blob store kept its path or descriptor in the public field `store::File.pathlike`. Every consumer read it and opened the file itself. The path is now a private tagged value: lazy (what `Bun.file()` makes) or pinned (a path plus the size and mtime the file must still have). Consumers use accessors: - `lazy_pathlike()` / `lazy_path()`: what a reader of a `Bun.file()` opens. `None` for a pinned file. - `fd()`: the descriptor of a `Bun.file(fd)`. - `display_path()`: for a name, an inspect string, an error message, or a stat of the path. Never for a read. - `pinned()`: the size and the `matches(&Stat)` check of a pinned file. `Store::get_path` and `Blob::store_path` are split into `path_for_display` and `path_for_open`, so each caller states what it does with the path. Nothing constructs a pinned store yet, so no behaviour changes. Each consumer keeps its code on the lazy arm and gains an arm that refuses a pinned file with EPERM on its existing error channel. `resolve_file_stat` is split into `stat_file` and `apply_file_stat`: its path and descriptor arms were identical.
Collaborator
Author
Nothing pins a file yet, so the variant carries an expect(dead_code) and PinnedFile holds the path alone. The compare fields and the constructor come with their first user. PathOrFileDescriptor::is_path and is_fd lost their last callers to the accessors.
Collaborator
Author
|
Closing: this is now part of #43896. A review of the stack found that this PR alone adds lines that nothing can run: the pinned variant had no constructor, and every pinned arm was unreachable until #43896. The accessor set is smaller there. A write or a metadata call reads the path through one accessor and has no pinned arm, so |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Behaviour change: none
Problem
store::File.pathlike(src/jsc/webcore_types.rs). 112 uses read it, and 18 of them open the file themselves.fs.openAsBlob()reads the file's new contents after it is modified instead of rejecting as Node does #43109 needs a file that a reader may open only after a check. With a public path, a reader that forgets the check reads the changed file. node:fs: make openAsBlob snapshot the file, fail reads after it changes, and stop sniffing the type #43112 checked in 6 of the 18 places.Fix
Bun.file()makes) or pinned. This PR has no constructor for a pinned store.lazy_pathlike(),lazy_path(),fd(),pinned(), anddisplay_path()for names and error text.get_pathandstore_pathsplit intopath_for_display()andpath_for_open().EPERMon its existing error channel.bun run rust:check-allpasses on 12 targets.Background
Blobpoints at a refcountedStore. ForBun.file()its data isstore::File.slice()shares the store.fs.openAsBlob()build a pinned store and adds the one verified open.Downsides
Bun.file(). 8 struct sizes, the file syscalls of 10 traced operations and the allocation counts are the same as on main (Notes).Notes
Measurements (release builds of main f385d9a and of #43896 at 8e9fccd, which holds this PR, linux x64; gdb
catch syscallbecause strace is not installed here):size_of(release):store::File128,store::Data136,Store176,ReadFile448,CopyFile480,NewSource<FileReader>512,FileResponseStream216,Blob104 B. All 8 are the same as on main. A const assert holds the path enum at the size ofPathOrFileDescriptor.Bun.file()syscalls: 10 of 10 traced operations make the same file syscalls as main:text()at 1 B and 64 KiB, a 300000-byte stream drain, fetch bodies of 1 KiB and 64 KiB over http, aBun.servehandler Response and a static route (GET and HEAD),Bun.writeof 1 MiB, aFormDatabody,Bun.spawnstdin.sendfileandcopy_file_rangeare still taken. The position of socketcloseandpread64calls varies from run to run, on main too.Bun.file(p)395.7 K on both builds. 10000 xFormDatabody 186.9 K on both (5 runs each. One run in 5 reads about 20 K lower, on either build).store::Fileoutside the store module. 26 calls of the named opt-outsdisplay_path()andpath_for_display().git merge-tree).Accessor choice per site:
display_path()(the named opt-out): inspect,name,SystemError.pathtext inReadFile,ReadFileUV,WriteFileWindows,CopyFile,CopyFileWindows,Blob.stat(), the static-route existence check inserver_body.rs, and S3 error text.path_for_open():FileRoute::onand theblob_store_pathhook thatimport()of ablob:URL reads through (jsc_hooks.rs,bundler/options.rs).fd(): every "is this aBun.file(fd)" test (do_close,is_allowed_to_close, stream de-duplication,mkdirpchecks).lazy_pathlike()/lazy_path(): every open,clonefile,uv_fs_copyfile,truncate,chmod,unlink, the FormData and TLS sync reads,Bun.spawnstdio, and shell interpolation.validate_writable_blobis the one check every write entry point runs (Bun.writedestination,writer(),unlink()), so it refuses a pinned file first.The pinned variant carries
#[expect(dead_code)]here, because nothing constructs it yet. #43896 adds the constructor, the compare fields and the verified open, and removes the attribute.PathOrFileDescriptor::is_path()andis_fd()are gone. Their last callers now ask the store (fd(),lazy_path()).Not in this PR, because the
dead_codelint rejects a field that nothing reads:Lazy::open_file_blobhanding back theStatit computes. #43896 adds it with its reader.Two redundant assignments in
CopyFile::run_asyncare gone (self.source_fd = ...pathlike.fd()after the same value was set at the top of the function), and an emptyifwith only a comment.Existing tests run on the debug build, all pass:
bun-file.test.ts,bun-file-read.test.ts,bun-file-fd-read.test.ts,bun-file-exists.test.js,bun-write.test.js,blob.test.ts,blob-write.test.ts,blob-file-name-ownership.test.ts,structured-clone-blob-file.test.ts,streams.test.js,FormData.test.ts,bun-serve-file.test.ts,bun-serve-static.test.ts,bun-serve-routes.test.ts,serve-file-slice-read-error.test.ts,fetch-file-upload.test.ts,tls-bunfile-leak.test.ts,bunshell-file.test.ts,image.test.ts,fs.test.ts, and the Blob andBun.filetests ofspawn.test.ts. Infs.test.tsthe testreaddirSync(path, {recursive: true}) should work x 100runs close to its own 10 s limit on a debug build and can exceed it, on any branch.rust:check-alltargets: linux gnu and musl (x64, arm64), android (x64, arm64), darwin (x64, arm64), windows msvc (x64, arm64), freebsd (x64, arm64).