fs: copy directory trees for fs.cp() on the thread pool - #65488
fs: copy directory trees for fs.cp() on the thread pool#65488codebytere wants to merge 2 commits into
Conversation
|
Review requested:
|
3065c10 to
6d25d6a
Compare
The C++ fast path that fs.cpSync() takes when no filter is given created the destination directories with default permissions, so a 0700 directory came out of the copy as 0755 (with the default umask). The JavaScript implementation, which fs.cp(), fs.promises.cp() and fs.cpSync() with a filter still use, chmod()s every directory it creates to the mode of its source, and so did cpSync before the port. Set the source directory's permissions on each directory the copy creates (the destination root included); directories that already exist keep theirs, as before. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
6d25d6a to
cce0cf5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65488 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01%
==========================================
Files 752 751 -1
Lines 252315 252791 +476
Branches 47444 47576 +132
==========================================
+ Hits 227395 227867 +472
+ Misses 16217 16206 -11
- Partials 8703 8718 +15
🚀 New features to boost your workflow:
|
73a8c02 to
0ecff36
Compare
0ecff36 to
508ece6
Compare
| if (error) { | ||
| return CpError::Std(error, ConvertPathToUTF8(dest_file_path)); | ||
| } | ||
| CpError inner = copy_dir_contents(entry_dir_path, dest_file_path); |
There was a problem hiding this comment.
An existing destination directory symlink can reach this recursive call. The new native path follows it and copies into its target, while the previous JS walk rejects with ERR_FS_CP_DIR_TO_NON_DIR.
small repro:
import fs from 'node:fs/promises';
import { join } from 'node:path';
import os from 'node:os';
const root = await fs.mkdtemp(join(os.tmpdir(), 'cp-'));
const src = join(root, 'src');
await fs.mkdir(join(src, 'dir'), { recursive: true });
await fs.writeFile(join(src, 'dir', 'file'), 'x');
for (const [name, filter] of [['native'], ['js', () => true]]) {
const dest = join(root, name);
const target = join(root, `${name}-target`);
await fs.mkdir(dest);
await fs.mkdir(target);
await fs.symlink(target, join(dest, 'dir'), 'dir');
try {
await fs.cp(src, dest, { recursive: true, filter });
console.log(name, 'success',
await fs.readFile(join(target, 'file'), 'utf8'));
} catch (err) {
console.log(name, err.code);
}
}on v24.7:
native ERR_FS_CP_DIR_TO_NON_DIR
js ERR_FS_CP_DIR_TO_NON_DIR
on this branch:
native success x
js ERR_FS_CP_DIR_TO_NON_DIR
This is an observable behaviour change: the previous async implementation rejects an existing destination directory symlink, while the native path follows it. ref #58869, is aligning the async API with cpSync() here intentional, or should the previous async behaviour be preserved?
There was a problem hiding this comment.
@jakecastelli not intentional, thanks - the intent was to take the native walk only where it behaves like the JS one, and an existing destination is exactly where the two differ (your #58869 list). 0c17f68 narrows it to the case where the destination directory doesn't exist yet (the mkDirAndCopy path); copying into an existing tree keeps the JS walk and all of its checks, so both halves of your repro reject with ERR_FS_CP_DIR_TO_NON_DIR again. The fresh-destination copy keeps the speedup (2 100 files: ~27 ms vs ~210 ms into an existing tree).
508ece6 to
0c17f68
Compare
fs.cp() and fs.promises.cp() walked the tree in JavaScript with several thread pool round trips per entry (opendir batches, two stat()s, the copyFile(), a chmod()), all awaited in sequence: a 2 100-file tree took ~215 ms with ~110 ms of that on the main thread, against ~36 ms for fs.cpSync(), which copies the tree in C++ when no filter is given. Factor that C++ walk into CopyDirRecursive(), which records the error instead of throwing so that it can run on any thread, and run it as one ThreadPoolWork request (CpDirJob) for fs.cp()/fs.promises.cp() when the destination directory does not exist yet and nothing has to run per entry (no filter, no dereference, permission model off). Copying into an existing tree keeps the JavaScript walk and its rules for what may already be there. Sockets, FIFOs and unknown entries found by the job are reported back to JavaScript, which rejects them with the same SystemErrors as before (cpSync keeps skipping them). The same tree now takes ~28 ms with under 1 ms on the main thread. The walk now uses the error_code overloads of std::filesystem throughout (directory iteration included), so an unreadable directory inside the tree is reported as EACCES by both cp() and cpSync() instead of terminating the process, which cpSync() has done since the walk moved to C++. Filesystem errors raised inside the walk keep their codes, with 'cp' as the syscall as cpSync reports them. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
0c17f68 to
6a197c9
Compare
Runs the directory walk of
fs.cp()/fsPromises.cp()as one thread pool request using the C++ implementationfs.cpSync()already has, instead of a JavaScript walk with several awaited round trips per entry; the first commit fixes that implementation giving created directories default permissions instead of the source directory's mode.benchmark/fs/bench-cp.js(new,fsPromises.cp()of 500 files), 30 runsfsPromises.cp()of a 2 100-file treecpSyncof a0700directory (umask 022)07550700The JavaScript walk does opendir batches, two
stat()s, thecopyFile()and achmod()per entry, each awaited in sequence, with the bookkeeping on the main thread.fs.cpSync()without afilterhas done the whole walk in C++ since #58461, but that walk creates directories with default permissions where the JavaScript walk (andcpSyncbefore the port) gives them the source directory's mode; the first commit fixes that (the mode is applied once the directory's contents are copied, so read-only source directories still copy), with a test that fails onmain.The second commit factors the walk into
CopyDirRecursive(), which records an error instead of throwing so it can run on any thread, and runs it as aThreadPoolWorkrequest when the destination directory does not exist yet and nothing has to run per entry (nofilter, nodereference, permission model off); copying into an existing tree keeps the JavaScript walk and every rule it has for what may already be there (#58869 lists wherecpSync's walk differs). Sockets, FIFOs and unknown entries found by the request are handed back to JavaScript, which rejects them with the sameSystemErrors as before (cpSynckeeps skipping them). The walk uses theerror_codeoverloads ofstd::filesystemthroughout, so an unreadable directory inside the tree is reported asEACCESby bothcp()andcpSync()wherecpSync()currently terminates the process; filesystem errors from inside the walk keep their codes and reportcpas the syscall, ascpSyncdoes.Refs: #58461
Tests: new
test-fs-cp-sync-directory-mode.mjs,test-fs-cp-async-special-files-in-tree.mjs(a socket and a FIFO inside the tree: rejected bycp(), skipped bycpSync(), same as onmain) andtest-fs-cp-unreadable-directory.mjs(aborts onmainforcpSync); alltest-fs-cp*pass; a differential run over the option matrix (dereference,verbatimSymlinks,preserveTimestamps,force/errorOnExist, fresh and pre-populated destinations, symlinks, a socket and a FIFO in the tree) produces the same trees and outcomes as before.Disclosure: the code, test, benchmark, measurements and this description were written by Claude Code, directed and reviewed by @codebytere.