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.copy, handle e.g. EBUSY #511

Closed
wants to merge 2 commits into from
Closed

Conversation

unkelpehr
Copy link

Hi!

The function copyFile in lib/copy/ncp.js assumes that the readStream was opened properly. When this assumption happens to be false, the writeStream associated with the operation will never close. Causing a lock on the "targetFilename" which in turn blocks (EPERM) all operations on "targetFilename" during the remainder of the application lifetime.

This is the relevant line in the ReadStream class method open. Notice that if an error occurred while opening the file, the stream is destroyed and an error is emitted. This is what copyFile doesn't handle.

It's hard to recreate this behaviour.. but in my test case, I monkey patched the ReadStream.prototype.open so that it fails with 'EBUSY' on the first call. Simulating an intrusive AV or whatnot.

This is the output when running the test case:

trying to copy file (attempt 1)
(opening read stream, simulating EBUSY)
an error occurred during copy, trying again in 1 sec
 { Error: 'EBUSY: resource busy or locked, open test/file.txt',
  code: 'EBUSY',
  path: 'test/file.txt' }

trying to copy file (attempt 2) // Using the unpatched version of ReadStream
an error occurred during copy, trying again in 2 sec
 { Error: EPERM: operation not permitted, open 'test/file-copy.txt' // <-- Destination is now locked
  errno: -4048,
  code: 'EPERM',
  syscall: 'open', // <-- Another attempt to open a read stream, this time using the original ReadStream.open.
                   //     It's probably lstat or something as a prelude to check if the target already exists.
  path: 'test/file-copy.txt' }

trying to copy file (attempt 3)
an error occurred during copy, trying again in 3 sec
 { Error: EPERM: operation not permitted, unlink 'test/file-copy.txt'
  errno: -4048,
  code: 'EPERM',
  syscall: 'unlink', // <-- This is ncp.js rmFile that's trying to remove the target file ({overwrite: true})
  path: 'test/file-copy.txt' }

trying to copy file (attempt 4) // <-- ncp.js rmFile, again, can't delete a file with an open write stream
trying to copy file (attempt 5) // <-- and again..
trying to copy file (attempt ..............) // <-- and again, and again, and again...

This commit simply waits for readStream to emit an open event (which happens after error checking) before initializing a write stream.

After commit:

trying to copy file (attempt 1)
(opening read stream, simulating EBUSY)
an error occurred during copy, trying again in 1 sec
 { Error: 'EBUSY: resource busy or locked, open test/file.txt',
  code: 'EBUSY',
  path: 'test/file.txt' }

trying to copy file (attempt 2)
succesfully copied file.txt

(I'm not that experienced in contributing to open source projects via GitHub, so I'm sorry beforehand if I've misunderstood this whole pull request thing =))

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 87.389% when pulling 4602945 on unkelpehr:master into 2599b67 on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 8, 2017

@unkelpehr This looks great!

Normally, I'd just merge this, but we're currently in the middle of significantly refactoring fs.copy. @manidlou Can you port these changes over to your refactored version of copy?

@manidlou
Copy link
Collaborator

manidlou commented Nov 8, 2017

@manidlou Can you port these changes over to your refactored version of copy?

Sure thing.

@RyanZim RyanZim mentioned this pull request Nov 8, 2017
6 tasks
@RyanZim RyanZim added this to the 5.0.0 milestone Nov 9, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Nov 9, 2017

@manidlou reapplied these changes to develop in #516; this will be released in v5.

@unkelpehr Thanks for all your work here! Closing since these changes are now in develop via b72ba64.

@RyanZim RyanZim closed this Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants