vfs: integrate with CJS and ESM module loaders#63653
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63653 +/- ##
==========================================
- Coverage 92.04% 90.26% -1.78%
==========================================
Files 381 741 +360
Lines 169208 241521 +72313
Branches 25926 45500 +19574
==========================================
+ Hits 155746 218007 +62261
- Misses 13174 15098 +1924
- Partials 288 8416 +8128
🚀 New features to boost your workflow:
|
|
@joyeecheung take a look, should be easier to review. |
Restore the "DO NOT depend on the patchability" warnings in esm/load.js and esm/resolve.js that were dropped along with the fs imports. The warning still applies; it now also points at node:vfs as one of the formal hook mechanisms callers should reach for instead. Addresses review feedback from @jsumners-nr in nodejs#63653
mcollina
force-pushed
the
vfs-module-loader-integration
branch
from
6321e08 to
51b033a
Compare
github-actions
Bot
removed
the
request-ci
joyeecheung
left a comment
joyeecheung
left a comment
There was a problem hiding this comment.
A design question recently occurred to me: have we explored the versioning of the mounting?
what do you mean? You mean multiple vfs layers on top of each other? |
For the stacks to have some kind of version number/ID to identify the current status? BTW I just noticed that there's no mention of |
I did purge them when doing the splitting; I forgot to bring them back. I'll add them to this PR. |
No but we totally should. |
Each VirtualFileSystem now exposes a per-process monotonically increasing `layerId`, assigned at construction. The id is stable across mount/unmount cycles for the lifetime of the instance and surfaces in: - debug() output for register / deregister so the layer stack is visible when NODE_DEBUG=vfs is enabled; - the overlap ERR_INVALID_STATE message, which now names the layer ids of the conflicting mounts. The id is the building block for tagging cache entries with the owning VFS, which a follow-up will use to replace the global loader-cache flush in deregisterVFS with a scoped purge. Refs: nodejs#63653 Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina
force-pushed
the
vfs-module-loader-integration
branch
from
51b033a to
294a19c
Compare
Replace the global loader-cache flush in deregisterVFS with a scope-purge that only drops entries owned by the unmounting VFS. Per-layer ownership is determined two ways: - For CJS-style filename-keyed caches (Module._cache, Module._pathCache, the CJS stat cache, the helpers.js realpath cache, and the package.json caches) entries are filtered with `vfs.shouldHandle(filename)`. __filename stays a clean absolute path so user code that does `path.dirname(__filename)` or similar is unaffected. - For the ESM cascaded loader's loadCache, entries are tagged at resolve time: when finalizeResolution() detects the resolved path is VFS-owned (via the new loaderGetLayerForPath hook), it appends `?vfs-layer=<id>` to the URL. The tag surfaces in `import.meta.url`, matching the cache-busting pattern used by HMR tooling. On deregister, entries whose URL carries the tag for the unmounting layer are deleted. Multi-mount setups no longer pay the cross-VFS cache-warmup penalty when a single VFS unmounts, and ESM modules loaded from a VFS become reachable for purge instead of leaking forever in the cascaded loader. New helpers exposed for VFS: - cjs/loader.js: clearStatCacheForVFS - helpers.js: purgeRealpathCacheForVFS, loaderGetLayerForPath - package_json_reader.js: purgePackageJSONCacheForVFS Adds test-vfs-scoped-cache-purge covering both the multi-mount isolation and the import.meta.url tag visibility. Refs: nodejs#63653 Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
@joyeecheung PTAL |
mcollina
force-pushed
the
vfs-module-loader-integration
branch
from
75bb5c7 to
99a5a5c
Compare
Co-authored-by: James M Snell <jasnell@gmail.com> Signed-off-by: Matteo Collina <hello@matteocollina.com>
Each VirtualFileSystem now tracks the absolute filenames it has handled (ownedFilenames) and the Module._pathCache keys that resolve to a VFS-owned filename (ownedPathCacheKeys). Recording happens at routing time inside the loader overrides (findVFSForStat / findVFSForRead / findVFSWith, the inline package.json override loops, and a pathCache write recorder installed on the cjs loader). On deregister, purgeLoaderCachesForVFS walks the per-VFS sets and removes entries from Module._cache, Module._pathCache, the stat / realpath / package.json caches in O(owned) instead of scanning every cache and calling vfs.shouldHandle() on each entry. The pathCache recorder uses a Set.has() lookup over activeVFSList so the per-write overhead is M cheap Set checks rather than M path normalizations. Also encapsulates the previous direct access to Module._cache / Module._pathCache from internal/vfs/setup.js behind the cjs loader's new purgeModuleCacheForVFS / setPathCacheWriteRecorder / cachePathResolution helpers, so the loader's private data structures stay owned by the loader module. Signed-off-by: Matteo Collina <hello@matteocollina.com>
clearStatCache no longer no-ops when statCache is null - it always (re)allocates a SafeMap, leaving the cache non-null and ready for use. Signed-off-by: Matteo Collina <hello@matteocollina.com>
A plain substring check on `vfs-layer=N` wrongly matched URLs carrying a tag whose layer ID has N as a numeric prefix (e.g. tag for layer 1 was also matched by 10, 11, ...). On unmount of layer 1 this caused the ESM load-cache entries of unrelated layers to be purged. Anchor the match on `?`/`&` before and `&`/`#`/end-of-string after so each tag matches its own layer only. Also: replace the brittle line-number reference to node_file.cc with the function name (BindingData::LegacyMainResolve). Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Measures the per-call dispatch cost the VFS layer adds to statSync/existsSync/accessSync/readFileSync against real-filesystem paths under 0, 1, 2, 5 and 10 mounted VFS layers. Signed-off-by: Matteo Collina <hello@matteocollina.com>
VirtualFileSystem#shouldHandle was recomputing both toNamespacedPath(resolve(mountPoint)) AND toNamespacedPath(resolve(inputPath)) on every fs call, for every active VFS. With N mounted layers this meant 2N path normalizations per fs op against a real-filesystem path that no VFS owns. Cache the normalized mount point on the VFS instance at mount time, and add a shouldHandleNormalized fast path that takes an already-normalized input. The setup.js dispatch helpers (findVFSForStat / findVFSForRead / findVFSForExists / findVFSForPath / findVFSWith / findVFSPackageJSON / legacyMainResolve / getLayerForPath) now normalize the input once before the walk and share it across every layer. Benchmarked with benchmark/vfs/bench-fs-dispatch.js on a real-filesystem target path under 0/1/2/5/10 mounted layers: op layers=10 before layers=10 after speedup statSync 96k ops/s 278k ops/s 2.9x existsSync 99k ops/s 315k ops/s 3.2x accessSync 98k ops/s 309k ops/s 3.2x readFileSync 42k ops/s 99k ops/s 2.4x Per-additional-layer cost drops from ~860 ns to ~80 ns for stat/exists/access (and from ~1700 ns to ~200 ns for readFile). The layers=0 (VFS not loaded) path is unaffected: vfsState.handlers stays null and fs ops short-circuit on a single null check. Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina
force-pushed
the
vfs-module-loader-integration
branch
4 times, most recently
from
8dc6d1c to
310bc02
Compare
Move VFS mounts into `${os.devNull}/vfs/layer-<id>/<prefix>`, a
reserved location that cannot exist on the real file system because
`os.devNull` is a character device on POSIX and a device-namespace
path on Windows - neither can have child filesystem entries. Virtual
paths never conflate with real paths, and the owning layer of a path
is decidable by inspecting the path itself.
The prefix passed to `vfs.mount(prefix)` becomes a purely logical name
inside the namespace - it is never resolved against `process.cwd()`.
`mount()` returns the resulting absolute mount point. Two instances
using the same prefix cannot collide, so overlap detection is gone.
Because the namespace makes ownership self-describing, this removes:
- the per-VFS `ownedFilenames` / `ownedPathCacheKeys` sets;
- the `Module._pathCache` write recorder;
- the `?vfs-layer=<id>` search-param tagging in the ESM resolver;
- the Windows drive-less-URL fixup in finalizeResolution.
Dispatch collapses to one normalization, one prefix check against the
namespace root, and a map lookup on the layer id - constant time in the
number of mounted layers. Unmount purges loader caches by a prefix scan
of the mount point, which fixes the two correctness gaps flagged in
review: symlink realpaths inside a VFS always resolve under the mount
point, so cached entries under any symlinked realpath are purged; and
resolved URLs are plain file URLs under the mount point, so
`import(import.meta.resolve(x))` hits the same module job on repeat.
`os.devNull` was chosen over `process.execPath` because it is
ASCII-clean by definition. This keeps mount points safe to embed in
dynamic-`import()` specifiers even when Node.js itself is installed
under a directory whose name contains URL-reserved characters (a case
exercised by the "linux-arm" CI job that runs from a path containing
`?` and other unusual characters).
`fs.statSync` / `fs.lstatSync` now catch ENOENT from the VFS handler
when `throwIfNoEntry` is false, matching the native binding contract.
The VFS handler surfaces ENOENT unconditionally for missing entries;
the fall-through to the real file system would otherwise traverse
`os.devNull` and produce ENOTDIR.
Docs, tests and benchmark adapted. `benchmark/vfs/bench-fs-dispatch.js`
now reports flat per-call latency across 1..10 mounted layers, versus
~80 ns/layer for stat/exists/access and ~200 ns/layer for readFile
before this change.
Refs: https://github.com/nodejs/single-executable/blob/main/docs/virtual-file-system-requirements.md#no-interference-with-valid-paths-in-the-file-system
Signed-off-by: Matteo Collina <hello@matteocollina.com>
mcollina
force-pushed
the
vfs-module-loader-integration
branch
from
310bc02 to
cefef62
Compare
|
@joyeecheung done, take a look |
`mount()` no longer accepts an argument. The user-supplied prefix
served no purpose beyond a cosmetic label - actual layer identity comes
from the per-instance `layerId`, so distinct instances never shared a
mount point even when they passed the same prefix. Removing the
argument eliminates the input validation, the escape check for `..`
segments, and the API-shape doubt about what a "logical prefix" means.
The `layer-` prefix on the layer segment of the mount path is likewise
gone: mount points are now `${os.devNull}/vfs/<id>` (for example
`/dev/null/vfs/0`) instead of `${os.devNull}/vfs/layer-0/<prefix>`.
The parser in `getLayerIdFromPath` simplifies to reading the digit
segment immediately after the VFS root.
Docs, tests and benchmark adapted. The dispatch benchmark still
reports flat per-call latency across 1..10 mounted layers.
Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
@joyeecheung ping. |
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
High level direction looks much better and more aligned with https://github.com/nodejs/single-executable/blob/main/docs/virtual-file-system-requirements.md#no-interference-with-valid-paths-in-the-file-system now, thanks, though I think the implementation still leaves quite a bit hard-codes and repeitions that should be cleaned up before landing to avoid taxing future changes to module loading with extra maintenance complexity & divergence..
| * @returns {object|undefined} | ||
| */ | ||
| function loaderReadPackageJSON(jsonPath, isESM, base, specifier) { | ||
| if (readPackageJSONOverride !== null) { |
There was a problem hiding this comment.
Helpers like this look like a lot of maintainance burden - every time someone adds a new binding, they have to update all these, and if they end up updating some internal parameters as part of a semver major change, it can cause another wave of churns for backports. Can you just use a helper to forward them all? Something like
function wrapLoaderMethod(originalFn) {
return function(...args) {
const override = overrideMap.get(originalFn);
if (override) return ReflectApply(override, this, args);
return ReflectApply(originalFn, this, args);
}
}(if a benchmark shows it actually matters for perf we could even switch on originalFn.length and build arity-based dispatches like processTicksAndRejections but I suspect the overhead is negligible for this path in the bigger picture). This would also shrink the diff here quite a bit.
| @@ -174,11 +219,26 @@ class VirtualFileSystem { | |||
| * @returns {boolean} | |||
| */ | |||
| shouldHandle(inputPath) { | |||
There was a problem hiding this comment.
I think this is dead code now?
| const parsed = JSONParse(content); | ||
| return { vfs, pjsonPath, parsed, sentinel: pjsonPath }; | ||
| } catch { | ||
| // SyntaxError or other errors, continue walking |
There was a problem hiding this comment.
This diverges from GetPackageJSON which would throw ERR_INVALID_PACKAGE_CONFIG on the first syntactically malformed package.json instead of keep walking, I think we should at least align with the actual behavior here or otherwise this can introduce very subtle bugs for users that create a tree into VFS and one of the package.json end up corrupted somehow.
What would be even better is to simply split the C++ GetPackageJSON into read + parse, and let the VFS side reuse the parsing logic, that also eliminates the duplication of serializePackageJSON etc and would make use of partial parsing with simdjson (we could technically just read the pacakge.json as buffer and avoid the UTF8 encoding cost too) instead of a wasteful full parse with JSON.parse.
| // mount. Treat that as JS (the caller will surface the real | ||
| // error when it later tries to load source). Propagate every | ||
| // other code (EACCES, ELOOP, etc). | ||
| if (e?.code === 'ENOENT') return 0; // EXTENSIONLESS_FORMAT_JAVASCRIPT |
There was a problem hiding this comment.
The original version normalizes all errors for EXTENSIONLESS_FORMAT_JAVASCRIPT, not just ENOENT.
| if (vfs.existsSync(filename) && vfsStat(vfs, filename) === 0) { | ||
| return { vfs, content: vfs.readFileSync(filename, options) }; | ||
| } | ||
| // Path inside mount but missing/not-a-file: synthesize ENOENT. Paths | ||
| // under the reserved namespace never exist on the real file system, | ||
| // so falling through would produce a confusing real-fs error. | ||
| throw createENOENT('open', filename); |
There was a problem hiding this comment.
| if (vfs.existsSync(filename) && vfsStat(vfs, filename) === 0) { | |
| return { vfs, content: vfs.readFileSync(filename, options) }; | |
| } | |
| // Path inside mount but missing/not-a-file: synthesize ENOENT. Paths | |
| // under the reserved namespace never exist on the real file system, | |
| // so falling through would produce a confusing real-fs error. | |
| throw createENOENT('open', filename); | |
| try { | |
| return { vfs, content: vfs.readFileSync(filename, options) }; | |
| } catch (e) { | |
| const code = e?.code; | |
| if (code === 'ENOENT' || code === 'EISDIR') { | |
| throw createENOENT('open', filename); | |
| } | |
| throw e; | |
| } |
I think you can simply do this here and there's not really much of a need to do exists/stat? One caveat is that the provider must implement EISDIR but that seems reasonable compared to the cost of doing multiple stats.
| if (content && content.length >= 4 && | ||
| content[0] === 0x00 && content[1] === 0x61 && | ||
| content[2] === 0x73 && content[3] === 0x6d) { | ||
| return 1; // EXTENSIONLESS_FORMAT_WASM |
There was a problem hiding this comment.
This shouldn't hard-code and should return internalBinding('constants').EXTENSIONLESS_FORMAT_WASM etc.
| } catch { | ||
| return -2; | ||
| } |
There was a problem hiding this comment.
| } catch { | |
| return -2; | |
| } | |
| } catch (err) { | |
| if (typeof err?.errno === 'number') return err.errno; | |
| throw err; | |
| } |
Collapsing all to a hard-coded -2 doesn't look quite right (also on Windows ENOENT is not necessarily -2).
| // 7-9: try pkgPath + ./index.ext | ||
| const mainExts = ['', '.js', '.json', '.node', | ||
| '/index.js', '/index.json', '/index.node']; | ||
| const indexExts = ['./index.js', './index.json', './index.node']; |
There was a problem hiding this comment.
Do we need to duplicate all these? It can go out of sync easily. legacyMainResolveExtensions is already (inappropriately) located in esm/resolve.js, I think we can simply share the arrays and do something like
legacyMainResolve(pkgPath, main, base) {
if (findVFS(pkgPath) === null) return undefined;
for (let i = 0; i < legacyMainResolveExtensions.length; i++) {
const shouldResolveByMain = i <= kMaxResolvedByMainIndex; // kResolvedByMainIndexNode
if (shouldResolveByMain && !main) continue;
const prefix = shouldResolveByMain ? main : '';
const candidate = join(pkgPath, prefix + legacyMainResolveExtensions[i]);
if (findVFSForStat(candidate)?.result === 0) return i;
}
const initial = main ? join(pkgPath, main) : join(pkgPath, 'index.js');
throw new ERR_MODULE_NOT_FOUND(initial, base, undefined);
}| StringPrototypeEndsWith(currentDir, '\\node_modules')) { | ||
| break; | ||
| } | ||
| const pjsonPath = join(currentDir, 'package.json'); |
There was a problem hiding this comment.
Since dirname/join preserves the prefix if it's absolute, I think can just do something like if (!vfs.shouldHandleNormalized(currentDir)) break; to save on the repeated normalization in stat/read etc.
| } else { | ||
| filePath = resolved; | ||
| } | ||
| const vfs = findVFS(filePath); |
There was a problem hiding this comment.
Nit: findVFS discards the normalized filePath which is a bit wasteful, if it returns that back we can reuse that in e.g. findVFSPackageJSON.
7 participants
Makes
require()andimportresolve files served bynode:vfs. Before this PR, mounted VFS files were only visible throughfs.*; the loaders went straight to the real filesystem.Design
vfs.mount()takes no arguments and returns the reserved absolute mount point of the instance:${os.devNull}/vfs/<layerId>(for example/dev/null/vfs/0).os.devNullis a character device on POSIX and a device-namespace path on Windows; neither can have child filesystem entries, so no real path can ever exist under this root.Everything about ownership is decidable from the path alone:
benchmark/vfs/bench-fs-dispatch.js) reports flat per-call latency across 1..10 mounted layers.realpathSyncof a VFS entry always resolves to another path under the same mount point, so cache entries hidden behind symlinks are captured by the prefix scan.file:URLs under the mount point;import(import.meta.resolve(x))re-hits the same module job.Two instances mounting simultaneously never collide (each gets its own
layer-<id>segment), so there is no overlap validation and no ordering hazard between mounts.Loader integration
Toggleable wrappers in the loaders. Null fast-path when no VFS is mounted; otherwise the VFS answers
stat/readFile/realpath/legacyMainResolve/getFormatOfExtensionlessFileand the fourpackage.jsonC++-binding calls.Module identity follows the path:
__filename,module.filename, andimport.meta.urlare the plain absolute path (orfile:URL) of the module under the mount point — no synthetic decorations.Review guide
Suggested reading order:
lib/internal/vfs/router.jsgetVfsRoot,getLayerRoot,getLayerIdFromPath.lib/internal/vfs/file_system.jsmount()returns the layer's reserved mount point.lib/internal/vfs/setup.jsfindVFS(O(1) lookup), fs handler, loader overrides with parity tosrc/node_modules.cc/src/node_file.cc, prefix-scan cache purge.lib/internal/modules/helpers.jsloader*wrappers,setLoaderFsOverrides/setLoaderPackageOverrides,purgeRealpathCacheForPrefix.lib/internal/modules/cjs/loader.jsstat()+ TS read routed through wrappers;purgeModuleCachesForPrefixfor unmount.lib/internal/modules/esm/resolve.jslegacyMainResolve+internalModuleStat+toRealPathrouted. No URL decoration.lib/internal/modules/esm/load.jsgetSourceSyncreads via the wrapper.lib/internal/modules/esm/get_format.jslib/internal/modules/package_json_reader.jspurgePackageJSONCacheForPrefix.lib/fs.jsstatSync/lstatSynchonourthrowIfNoEntry:falseon ENOENT from the VFS handler.doc/api/vfs.mdTests (all gated by
--experimental-vfs):test-vfs-mount,test-vfs-mount-errors,test-vfs-multi-mount,test-vfs-require,test-vfs-import,test-vfs-module-hooks,test-vfs-module-hooks-cleanup,test-vfs-package-json,test-vfs-package-json-cache,test-vfs-invalid-package-json,test-vfs-scoped-cache-purge,test-vfs-layer-id,test-vfs-layer-tag-prefix.Refs
The reserved-namespace design follows the "no interference with valid paths in the file system" requirement from the SEA VFS requirements doc.
Out of scope
SEA + VFS, overlay/stacking of multiple VFS layers under one prefix, migrating the C++
package_configs_cache, broader permission-model integration.