fs: add fs.copyFile{Sync} by cjihrig · Pull Request #15034 · nodejs/node · GitHub

This PR adds fs.copyFile() and fs.copyFileSync().

Ignore the first commit, which updates libuv. This PR depends on libuv functionality that hasn't landed in Node yet. This PR is blocked until deps: upgrade libuv to 1.14.1 #14866 lands - a few things are needed for a new libuv release.
The test currently crashes. Again, this needs a fix in libuv, but can be worked around in Node if that doesn't land. So, don't even kick off a CI run.
Needs documentation.

Fixes: #14906

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

fs

Since these are already being checked on the js side, perhaps just a CHECK in here?

The JS layer checks don't actually verify this.

ugh.. that's right. hmmm. we should definitely fix that.
would much prefer any new errors to go through the internal/errors path.

I agree. fs.js in general needs some TLC.

I'll see if I can schedule some time next week to work on that. The risk, of course, is that fs is one of the most-monkey-patched modules out there and we risk breaking a lot of people by being too aggressive.

fs is one of the most-monkey-patched modules out there and we risk breaking a lot of people by being too aggressive.

For new methods in fs it might then make sense to be more aggressive rather than not. Personally, the more assertions the better. Keeps things sharp.

Are we certain this should be the default? I don't want to see bug reports later about how Node.js made it so that their files were all screwed up... and there is a non-zero chance of abusing this.

Yes. cp defaults to overwriting, as does our own fs.rename().

Yeah, I'm aware, just makes me rather cringy. Guess I'll grit my teeth and bear it.

So far this is looking great. I assume the libuv commit is temporary?

Perhaps add a note that makes it clear that the default behavior is to overwrite?

Also, a simple example is always helpful.

I assume the libuv commit is temporary?

Yes, I explained in more detail in the original post.

ha! if that first bullet was already there and I just missed it, forgive me :-)

I would love to see a code example in the docs

There will be one.

Oh, btw, @cjihrig ... have you tested what happens if there's a file read or write error in the middle of the copy operation? Say, for instance, if the device goes down during?

any possible way to observe copy progress?

@jasnell So, there are few platform specific underlying implementations in libuv. It's possible that Windows' CopyFile() might do something different from macOS' copyfile() in that case. But, in libuv, there is a sendfile() fallback that attempts to delete the destination file if anything goes wrong during the actual copy.

@YurySolovyov no, not currently.

@cjihrig ... ok, thanks for the clarification there. It may be something that we need to watch for across platforms so we can document any differences in behavior. In particular, I want to be clear about whether or not the operation is atomic.

I would prefer to make no guarantees of atomicity, especially at the level of Node. The platform specific calls could change without our knowledge, and there is at least one non-atomic path through libuv (see warning here). I think it's fair to say we'll make a best effort to remove the destination file if something goes wrong.

+1 ... can you, perhaps, add a comment to that effect in the docs?

@jasnell this should be ready now. The libuv update is complete, and the temporary commit removed. I think I've added all of the examples and documentation notes that you requested.

Did we ever reach a conclusion whether to include require()s in examples? The fs API does both at the moment.

I updated the code samples to include the require()s

The test does not appear to work on v8.x

Path: parallel/test-fs-copyfile
assert.js:41
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: undefined === 'ERR_INVALID_ARG_TYPE'
    at Object.<anonymous> (/Users/mborins/code/node/master/test/common/index.js:703:12)
    at Object.<anonymous> (/Users/mborins/code/node/master/test/common/index.js:509:15)
    at expectedException (assert.js:589:19)
    at innerThrows (assert.js:623:21)
    at Function.throws (assert.js:637:3)
    at Object.expectsError (/Users/mborins/code/node/master/test/common/index.js:737:12)
    at Object.<anonymous> (/Users/mborins/code/node/master/test/parallel/test-fs-copyfile.js:63:8)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)

Would you be willing to manually backport?

Fixes: nodejs#14906 PR-URL: nodejs#15034 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Backport in #15322

Fixes: #14906 PR-URL: #15034 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Fixes: nodejs#14906 PR-URL: nodejs#15034 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Aug 25, 2017

cjihrig added fs Issues and PRs related to the fs subsystem / file system. and removed libuv Issues and PRs related to the libuv dependency or the uv binding. labels Aug 25, 2017

cjihrig force-pushed the copyfile branch from a12bd0e to 2574341 Compare August 25, 2017 18:31

jasnell reviewed Aug 25, 2017

View reviewed changes

jasnell added the wip Issues and PRs that are still a work in progress. label Aug 25, 2017

jasnell reviewed Aug 25, 2017

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 25, 2017

benjamingr reviewed Aug 30, 2017

View reviewed changes

cjihrig force-pushed the copyfile branch from 2574341 to 23ae179 Compare September 6, 2017 16:54

cjihrig changed the title WIP: fs: add fs.copyFile{Sync} fs: add fs.copyFile{Sync} Sep 6, 2017

cjihrig force-pushed the copyfile branch 2 times, most recently from e5af78e to a070509 Compare September 6, 2017 17:42

TimothyGu removed the wip Issues and PRs that are still a work in progress. label Sep 7, 2017

TimothyGu approved these changes Sep 7, 2017

View reviewed changes

jasnell approved these changes Sep 7, 2017

View reviewed changes

benjamingr approved these changes Sep 7, 2017

View reviewed changes

tniessen approved these changes Sep 7, 2017

View reviewed changes

cjihrig force-pushed the copyfile branch from a070509 to 72ae85a Compare September 7, 2017 13:45

cjihrig force-pushed the copyfile branch from 6580d02 to 11b7428 Compare September 8, 2017 04:08

cjihrig merged commit 11b7428 into nodejs:master Sep 8, 2017

cjihrig deleted the copyfile branch September 8, 2017 04:09

This was referenced Sep 8, 2017

Provide a way to create clone/CoW copies #12902

Closed

test: do not write fixture in test-require-symlink #15067

Closed

MylesBorins added the backport-requested-v8.x label Sep 10, 2017

cjihrig mentioned this pull request Sep 10, 2017

Backport fs.copyFile{Sync} to v8.x #15322

Closed

4 tasks

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Sep 10, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

segayuu mentioned this pull request Sep 19, 2017

[perfomance] Support native copyFile. hexojs/hexo-fs#8

Closed

MylesBorins mentioned this pull request Jan 15, 2018

Semver Minor Tracking nodejs/Release#298

Closed

gibfahn added the lts-watch-v6.x label Jan 15, 2018

coderabbitai Bot mentioned this pull request Jul 28, 2026

Fix/windows agent teams native panes stablyai/orca#11106

Open

5 tasks

coderabbitai Bot mentioned this pull request Aug 19, 2026

Account & brain reliability: heal machine identity, auto-recover pairing, truthful failures, diagnosable errors arul28/ADE#1122

Merged

fs: add fs.copyFile{Sync} by cjihrig · Pull Request #15034 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: add fs.copyFile{Sync} - #15034

fs: add fs.copyFile{Sync}#15034cjihrig merged 1 commit intonodejs:masternodejs/node:masterfrom cjihrig:copyfilecjihrig/node:copyfileCopy head branch name to clipboard

Conversation

cjihrig commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 25, 2017

Uh oh!

jasnell commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 30, 2017

Uh oh!

YurySolovyov commented Aug 30, 2017

Uh oh!

cjihrig commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 30, 2017

Uh oh!

cjihrig commented Aug 30, 2017

Uh oh!

jasnell commented Aug 30, 2017

Uh oh!

cjihrig commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

cjihrig commented Sep 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

fs: add fs.copyFile{Sync}#15034
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:copyfile

cjihrig commented Aug 25, 2017 •
edited
Loading

cjihrig commented Aug 30, 2017 •
edited
Loading