Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: add fs.copyFile{Sync} #15034

Merged
merged 1 commit into from
Sep 8, 2017
Merged

fs: add fs.copyFile{Sync} #15034

merged 1 commit into from
Sep 8, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 25, 2017

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

@nodejs-github-bot nodejs-github-bot added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Aug 25, 2017
@cjihrig 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
Environment* env = Environment::GetCurrent(args);

if (!args[0]->IsString())
return TYPE_ERROR("src must be a string");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JS layer checks don't actually verify this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


* `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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

jasnell commented Aug 25, 2017

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

doc/api/fs.md Outdated

`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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, a simple example is always helpful.

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 25, 2017

I assume the libuv commit is temporary?

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

@jasnell
Copy link
Member

jasnell commented Aug 25, 2017

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

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 25, 2017
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see a code example in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be one.

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

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
Copy link

any possible way to observe copy progress?

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 30, 2017

@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
Copy link
Member

jasnell commented Aug 30, 2017

@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
Copy link
Contributor Author

cjihrig commented Aug 30, 2017

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
Copy link
Member

jasnell commented Aug 30, 2017

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

@cjihrig cjihrig changed the title WIP: fs: add fs.copyFile{Sync} fs: add fs.copyFile{Sync} Sep 6, 2017
@cjihrig cjihrig force-pushed the copyfile branch 2 times, most recently from e5af78e to a070509 Compare September 6, 2017 17:42
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 6, 2017

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

@TimothyGu TimothyGu removed the wip Issues and PRs that are still a work in progress. label Sep 7, 2017
doc/api/fs.md Outdated
following example.

```js
const { COPYFILE_EXCL } = fs.constants;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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?

cjihrig added a commit to cjihrig/node that referenced this pull request Sep 10, 2017
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 cjihrig mentioned this pull request Sep 10, 2017
4 tasks
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 10, 2017

Backport in #15322

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
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>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
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>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
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>
MylesBorins added a commit that referenced this pull request Sep 12, 2017
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
MylesBorins added a commit that referenced this pull request Sep 12, 2017
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
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add fs.copyFile for copying files, using uv_fs_copyfile