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

Add create/createNew options to Deno.copyFile #4584

Closed
wants to merge 6 commits into from

Conversation

dubiousjim
Copy link
Contributor

We add create and createNew options to copyFile, paralleling those for open and writeFile. See #4569, which discusses how this relates to some concurrent PRs, and also compares to peer languages. As mentioned there, Deno.copyFile with createNew: true corresponds to Node's fs.copyFile with the COPYFILE_EXCL flag, and to the POSIX shell command cp -Tn.

This is part of larger series of filesystem commits (#4017).

  • When possible, we use the fastest implementation provided by Rust (std::fs::copy; create must be true and createNew false). In other cases, for the operations to still be atomic, we need to open two files and copy the data using std::io::copy. That may be slower because it requires copying data out of kernel space.

  • Note for the future: if we ever shift to a tokio::fs version of op_copy_file (see refactor: ops/fs.rs should use tokio::fs where applicable #4188), I discovered that Tokio's implementation of this always uses the slower path, via tokio::io::copy. It also wasn't copying permissions correctly. (See tokio::fs::copy sometimes not copying permissions? tokio-rs/tokio#2341.)

    (PRs I plan to propose later make use of some Unix-specific functionality when available, which means bypassing Tokio here anyway.)

  • As with open in Cleanup Deno.open and Deno.writeFile #4580, this PR makes a special effort to ensure that the errors triggered by createNew: true are always AlreadyExists. We also ensure that the errors triggered by create: false are always NotFound. In other cases, we let the different platforms generate their different errors. (For example, on Unix attempting to copy a file onto a directory gives a "Is a directory" error, but on Windows it's a PermissionDenied.)

  • This PR also makes an effort to ensure that when the source path is a directory, we get a predictable error, regardless of the presence of create or createNew options governing the destination path. That is, the check on the source takes priority.

    (I didn't set out aiming for any specific pattern of errors. Just hoped to find minimal interventions that did the most towards cleaning up the messy complexity we were getting of which errors were reported how, before this PR, and then the different messy complexity we were getting after introducing the other changes in this PR. I think I found a good balance. It involves changing one Unix error and one Windows error. See comments in op_copy_file.)

  • As with writeFile in Cleanup Deno.open and Deno.writeFile #4580, copyFile has a very simple version of the internal checkOpenOptions function, which one may be tempted to inline. I left it factored out because later PRs which we might consider expand would expand this function.

  • We refactor some copyFile tests to avoid repetition (assertFile function).

  • We add tests of the new create and createNew functionality. We also add tests documenting the different errors that Windows and Unix give when trying to copy onto directories. (It would be good to know if/when we ever build on a system where these are different.) And verifying that the errors are always AlreadyExists if triggered by createNew being true.

  • We haven't yet implemented creating symlinks on Windows. On Unix, we verify that copyFile gives an error when writing to a path that contains any valid symlink when createNew is true, and that the error is always AlreadyExists. Also that it gives a (different) error when createNew is false, but writing to a path that contains a symlink to a directory. If the options are { create: false, createNew: false }, and the target is a dangling symlink, the error is `NotFound.

  • The underlying Rust machinery we rely on doesn't give an error when create or createNew are true, but writing to a path that contains a dangling symlink. Instead it creates a new file at the target destination, and copies the file there. (It "copies through" the dangling symlink.) Python's shutil.copy and Node's fs.copyFileSync also do this. The shell cp operation on the other hand, won't perform the copy if the destination is a dangling symlink. We'd have to make special efforts (with runtime cost) to match the shell, and I haven't done that here.

  • Some of the copyFile tests in our codebase rely on the older behavior of writeFile wrt mode, and need to be updated after Cleanup Deno.open and Deno.writeFile #4580 lands. Whichever of this PR or Cleanup Deno.open and Deno.writeFile #4580 is merged first, I can push a commit to the other PR that updates those copyFile tests.

These work as for writeFile. The fastest implementation is used when {
create: true, createNew: false } (this is the default, and matches
the older implementation).

To implement createNew atomically, we have to fall back to what I think
is a slower implementation, where the data gets copied from kernel space
to user space, and back.

Also tweaks a debug! call, and a comment.

In some cases on Windows, we convert PermissionDenied -> AlreadyExists.
@dubiousjim
Copy link
Contributor Author

cc @cknight, @ry

@bartlomieju bartlomieju added the cli related to cli/ dir label May 15, 2020
@bartlomieju
Copy link
Member

@dubiousjim please resolve conflicts

@ry
Copy link
Member

ry commented Jun 25, 2020

Hey @dubiousjim, very sorry for the slow review. This behavior can be done in JS with the existing ops. Your claim is that this is atomic, but it isn't. Other processes on the system may be able to move/manipulate files in between the various syscalls in your revised copy op. Thanks for the effort but closing without merge.

@ry ry closed this Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants