fs: add fs.copyFile{Sync} #15034

cjihrig · 2017-08-25T16:35:40Z

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

jasnell · 2017-08-25T18:49:20Z

src/node_file.cc

+  Environment* env = Environment::GetCurrent(args);
+
+  if (!args[0]->IsString())
+    return TYPE_ERROR("src must be a string");


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.

jasnell · 2017-08-25T18:53:00Z

doc/api/fs.md

+
+* `src` {string|Buffer|URL} source filename to copy
+* `dest` {string|Buffer|URL} destination filename of the copy operation
+* `flags` {number} modifiers for copy operation. **Default:** `0`


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.

jasnell · 2017-08-25T19:02:08Z

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

jasnell · 2017-08-25T19:03:26Z

doc/api/fs.md

+
+`flags` is an optional integer that specifies the behavior
+of the copy operation. The only supported flag is `fs.constants.COPYFILE_EXCL`,
+which causes the copy operation to fail if `dest` already exists.


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

Also, a simple example is always helpful.

cjihrig · 2017-08-25T19:04:36Z

I assume the libuv commit is temporary?

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

jasnell · 2017-08-25T19:29:26Z

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

benjamingr · 2017-08-30T18:14:50Z

doc/api/fs.md

@@ -741,6 +741,40 @@ Returns an object containing commonly used constants for file system
 operations. The specific constants currently defined are described in
 [FS Constants][].

+## fs.copyFile(src, dest[, flags], callback)


I would love to see a code example in the docs

There will be one.

jasnell · 2017-08-30T19:20:12Z

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?

YurySolovyov · 2017-08-30T19:23:11Z

any possible way to observe copy progress?

cjihrig · 2017-08-30T19:24:50Z

@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.

jasnell · 2017-08-30T19:45:51Z

@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.

cjihrig · 2017-08-30T20:20:22Z

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.

jasnell · 2017-08-30T20:50:54Z

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

cjihrig · 2017-09-06T17:43:41Z

@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.

tniessen · 2017-09-07T11:53:22Z

doc/api/fs.md

+following example.
+
+```js
+const { COPYFILE_EXCL } = fs.constants;


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

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>

MylesBorins · 2017-09-10T02:51:11Z

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>

cjihrig · 2017-09-10T20:57:47Z

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

fs: add fs.copyFile{Sync}

11b7428

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>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add fs.copyFile{Sync} #15034

fs: add fs.copyFile{Sync} #15034

cjihrig commented Aug 25, 2017 •

edited

Loading

jasnell Aug 25, 2017

cjihrig Aug 25, 2017

jasnell Aug 25, 2017

cjihrig Aug 25, 2017

jasnell Aug 25, 2017

jorangreef Sep 8, 2017

jasnell Aug 25, 2017

cjihrig Aug 25, 2017

jasnell Aug 25, 2017

jasnell commented Aug 25, 2017

jasnell Aug 25, 2017

cjihrig commented Aug 25, 2017

jasnell commented Aug 25, 2017

benjamingr Aug 30, 2017

cjihrig Aug 30, 2017

jasnell commented Aug 30, 2017

YurySolovyov commented Aug 30, 2017

cjihrig commented Aug 30, 2017 •

edited

Loading

jasnell commented Aug 30, 2017

cjihrig commented Aug 30, 2017

jasnell commented Aug 30, 2017

cjihrig commented Sep 6, 2017

tniessen Sep 7, 2017

cjihrig Sep 7, 2017

MylesBorins commented Sep 10, 2017

cjihrig commented Sep 10, 2017

fs: add fs.copyFile{Sync} #15034

fs: add fs.copyFile{Sync} #15034

Conversation

cjihrig commented Aug 25, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 25, 2017

Choose a reason for hiding this comment

cjihrig commented Aug 25, 2017

jasnell commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 30, 2017

YurySolovyov commented Aug 30, 2017

cjihrig commented Aug 30, 2017 • edited Loading

jasnell commented Aug 30, 2017

cjihrig commented Aug 30, 2017

jasnell commented Aug 30, 2017

cjihrig commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Sep 10, 2017

cjihrig commented Sep 10, 2017

cjihrig commented Aug 25, 2017 •

edited

Loading

cjihrig commented Aug 30, 2017 •

edited

Loading