Update dest atime after copy/copy-sync #633

mbargiel · 2018-10-12T20:04:27Z

This PR provides a fix for the recently failing "copy-preserve-timestamps" (sync & async) tests on MacOS (partial fix for #571). (I believe they have been incorrect for a while but passed by chance due to timing and the coarse 1-s resolution time of HFS+.)

The main observation is that after copying a source file, the atime of that file may be modified (probably always will, due to the copy operation implicitly updating the atime when it reads from the file according to https://nodejs.org/docs/latest-v8.x/api/fs.html#fs_stat_time_values), so if we want to provide a true preserveTimestamps option, we need to re-read the source atime and use that value when updating the destination timestamps.

mbargiel · 2018-10-12T20:05:34Z

lib/copy-sync/__tests__/copy-sync-preserve-time.test.js

@@ -19,11 +19,18 @@ describeIfPractical('copySync() - preserveTimestamps option', () => {
  let TEST_DIR, SRC, DEST, FILES

  beforeEach(done => {
-    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-time')
+    TEST_DIR = path.join(os.tmpdir(), 'fs-extra', 'copy-sync-preserve-timestamp')


Just making this identical to its async counterpart.

mbargiel · 2018-10-12T20:06:03Z

lib/copy-sync/__tests__/copy-sync-preserve-time.test.js

    SRC = path.join(TEST_DIR, 'src')
    DEST = path.join(TEST_DIR, 'dest')
    FILES = ['a-file', path.join('a-folder', 'another-file'), path.join('a-folder', 'another-folder', 'file3')]
-    FILES.forEach(f => fs.ensureFileSync(path.join(SRC, f)))
+    const timestamp = Date.now() / 1000 - 5


If you prefer, I could just hard-code a date in the past rather than use "now minus 5 seconds".

mbargiel · 2018-10-12T20:07:16Z

lib/copy-sync/__tests__/copy-sync-preserve-time.test.js

-          assert.strictEqual(toStat.mtime.getTime(), utimes.timeRemoveMillis(fromStat.mtime.getTime()))
-          assert.strictEqual(toStat.atime.getTime(), utimes.timeRemoveMillis(fromStat.atime.getTime()))
-        }
+        // Windows has sub-second support: https://github.com/nodejs/io.js/issues/2069


I updated the comment after checking the nodejs/node#2069 thread - according to its conclusion, we should no longer need to RemoveMillis for Windows (so maybe I should have switched the conditions around instead, or what...? I'm open to reviewing this section)

Update - I realized afterwards that utimes.utimesMillisSync properly implements sub-second timestamp changes by using fs.futimes rather than fs.utimes - that looks like a bug in Node's fs.utimes function.

Still, I updated the comment and code since the bug reported by nodejs/node#2069 has been fixed years ago, so it seems safe to no longer have a different check for Windows. Tests will tell us if it's valid (I can't check on Windows atm)

mbargiel · 2018-10-15T17:53:42Z

The lint failure is caused by #631, and fixed in #636.

EDIT: no longer relevant after rebasing on top of f3a2eed

mbargiel · 2019-05-14T20:13:30Z

Rebased on top of latest master - plus added a fix when the source file is read-only.

coveralls · 2019-05-14T20:18:05Z

Coverage increased (+0.2%) to 82.151% when pulling d09b23b on mbargiel:fix/copy-preserve-timestamps-atime into a6b1a44 on jprichardson:master.

mbargiel · 2019-05-14T20:19:43Z

lib/copy/copy.js

+    return fs.chmod(dest, srcStat.mode, cb)
+  }
+
+  const next = (err) => {


This part is a bit tricky. (Check the sync version to better understand.)

Basically, the flow is:
1 -> (optional) set writable if not writable
2 --> re-read atime from source
3 ---> set dest atime
4 ----> set dest mode

The complexity stems from the optional step. I'm open to suggestions for making this more readable.

RyanZim · 2019-05-14T20:22:36Z

Initial quick review LGTM; but I'd like @manidlou to take a detailed look, since I'm not very familiar with this part of the codebase.

mbargiel · 2019-05-14T20:29:33Z

I checked the coverage loss.

In the case of copySync, I expanded an else branch in a function that does not seem covered (copyFileFallback). (https://coveralls.io/builds/23386203/source?filename=lib%2Fcopy-sync%2Fcopy-sync.js#L101)
In the case of copy, I modified setDestModeAndTimestamps and introduced a few additional callback-based operations, and there are no tests that cover the true branch if the usual if (err) return cb(err) pattern. I would have to set up a couple extra tests where each intermediate step fails, let me know if this is required. (https://coveralls.io/builds/23386203/source?filename=lib%2Fcopy%2Fcopy.js#L112)

manidlou · 2019-05-26T05:59:24Z

lib/copy-sync/copy-sync.js

-      return utimesSync(dest, srcStat.atime, srcStat.mtime)
+      // Make sure the file is writable first
+      if ((srcStat.mode & 0o200) === 0) {
+        fs.chmodSync(dest, srcStat.mode | 0o200)


This seems unnecessary! we are chmoding dest at the end anyway.

Actually, it is required. One of the tests I added reveals that it doesn't work without this if the source file is read-only (also manually tested on both Windows and Mac).

If src is readonly, then so is dest after fs.copyFileSync(src, dest)

utimesSync then fails with a permission error because it's not allowed to open the dest file for write.

Simple repro on Node 8.11.3, with latest head (9f1c029). Assumed to be run from the fse directory.

const fse = require('.') fse.ensureFileSync('src') fse.chmodSync('src', 0x111) fse.copySync('src', 'destNoPreserve') // works fse.copySync('src', 'destPreserve', { preserveTimestamps: true }) // fails with the following stack trace (Windows example): Error: EPERM: operation not permitted, open 'C:\Users\maximeb\code\node-fs-extra\destPreserve' at Object.fs.openSync (fs.js:646:18) at utimesMillisSync (C:\Users\maximeb\code\node-fs-extra\lib\util\utimes.js:68:17) at copyFile (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:79:7) at onFile (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:55:25) at getStats (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:50:44) at startCopy (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:40:10) at handleFilterAndCopy (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:35:10) at Object.copySync (C:\Users\maximeb\code\node-fs-extra\lib\copy-sync\copy-sync.js:28:10)

With this proposed change, no exception. If I comment out line 74, then I get the same exception.

If you don't want to keep this as-is, I see a few options:

Make sure copyFileSync does not set the dest mode to be the same as src

Change utimesMillisSync to be more robust if it fails with EPERM

Change utimesMillisSync to avoid this problem by not using fs.openSync(<path>, 'r+') + fs.futimesSync but straight out fs.utimesSync which works even on readonly files (note: I'm not sure the futimes and utimes interfaces are fully compatible, this might be the reason for using futimes in utimesMillisSync in the first place...)

Change this code to implement the chmod in a try/catch around utimesSync... but then it's no simpler than an if, just more costly.

Any other idea is also welcome.

lib/copy-sync/copy-sync.js

lib/copy/copy.js

manidlou · 2019-05-26T06:23:55Z

lib/copy-sync/copy-sync.js

+  if (opts.preserveTimestamps) {
+    const updatedSrcStat = fs.statSync(src)
+    fs.futimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
+  }



Also I just realized we forgot to chmod again here before closing fds. copy does that and we need to be consistent. So I think it is better we have the same function setDestModeAndTimestamps() like in copy so that we can call it after both native copy and fallback.

That would make sense. I'll try it out.

Question, though: if the fallback copy is used, wouldn't it be much better to use the futimes and fchmod operations directly to make the operation both atomic and faster, rather than closing the file descriptor and calling the path-based setDestModeAndTimestamps() function?

In fact, the setDestModeAndTimestamps function could be split in two, one taking paths in, and the other taking file descriptors in. Something like:

copyFile -> fs.copy -> setDestModeAndTimestamps(src, dest...) -> setDestModeAndTimestampsFd(fsd, fwd...)

copyFileFallback -> fs.open -> (copy) -> setDestModeAndTimestampsFd(fsd, fwd...) -> fs.close

What do you think?

Not sure the latter works for async since the async copyFileFallback uses streams with implicit FDs. But it seems a good solution for the sync version.

@manidlou What do you think of f55125e?

I also added tests for copyFileFallback in 1a6eacb - perhaps in a rather unorthodox way, but I quickly figured that we should basically ensure feature parity between both implementations, so why not leverage the same tests...?

brodycj · 2019-05-31T20:13:43Z

This fix looks important, what is the status?

mbargiel · 2019-06-01T04:12:43Z

@brodybits I just pushed commits that hopefully address review feedback.

mbargiel · 2019-07-27T17:39:05Z

@manidlou What did you think of the changes I made after your initial review? (I will squash once approved.)

manidlou · 2019-07-30T06:01:15Z

so this is my suggestion:

do not set the dest mode when we create the dest
use native fs.utimes*() and fs.futimes*() (instead of our internal utimes implementation)
if we do the above, then we don't need to check if the file is writable and we can simplify the setDestTimestampsAndMode function like this, which is so much easier to read and understand. All tests also pass with this implementation. I tried your example too and that one worked as well meaning dests created successfully without any errors.

function setDestTimestampsAndMode (srcStat, src, dest, opts) {
  const utimesSync = typeof dest === 'string' ? fs.utimesSync : fs.futimesSync
  const chmodSync = typeof dest === 'string' ? fs.chmodSync : fs.fchmodSync
  if (opts.preserveTimestamps) {
    const updatedSrcStat = fs.statSync(src)
    utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
  }
  chmodSync(dest, srcStat.mode)
}

mbargiel · 2019-07-30T18:12:28Z

so this is my suggestion:

do not set the dest mode when we create the dest

use native fs.utimes*() and fs.futimes*() (instead of our internal utimes implementation)

if we do the above, then we don't need to check if the file is writable and we can simplify the setDestTimestampsAndMode function like this, which is so much easier to read and understand. All tests also pass with this implementation. I tried your example too and that one worked as well meaning dests created successfully without any errors.
function setDestTimestampsAndMode (srcStat, src, dest, opts) {
  const utimesSync = typeof dest === 'string' ? fs.utimesSync : fs.futimesSync
  const chmodSync = typeof dest === 'string' ? fs.chmodSync : fs.fchmodSync
  if (opts.preserveTimestamps) {
    const updatedSrcStat = fs.statSync(src)
    utimesSync(dest, updatedSrcStat.atime, updatedSrcStat.mtime)
  }
  chmodSync(dest, srcStat.mode)
}

That would certainly be more straightforward. Let me take a stab at that tonight.

mbargiel · 2019-07-30T21:21:42Z

~~@manidlou I think the problem lies in the following:~~

~~do not set the dest mode when we create the dest~~

The code actually relies on fs.copyFileSync(src, dest) (see here), which will do the full copy, including preserving the mode (though that could be OS-dependent? Not sure - the nodejs documentation on the topic is pretty thin on details). So while in theory I agree with you that your proposed implementation would be more straight-forward, in practice it can only be implemented if we stop relying on fs.copyFileSync and implement the copy operation from scratch - in other words, it makes sense in the "copyFileFallback" implementation.

~~EDIT: Ooooh. Nevermind. I indeed tested your proposal, and the key was using the native fs.[f]utimes* version. Tests do pass now. Alright, I'll fix it like that!~~

SECOND EDIT: I found an issue in my test implementation that (presumably) re-ran copy[Sync] tests with the fallback implementation: it didn't work as expected, and actually caused these tests to only run the Fallback path, twice. Fixing this surfaced a problem: the native fs.utimes truncates millis when passed Date objects, unlike the native fs.futimes which seems fine with millis. (At least, this is node v8.11.3 behavior.) This means that with your proposal, the preserveTimestamp tests fail for the fs.copyFile[Sync]-based path but not the copyFile[Sync]Fallback path.

(And passing the .getTime() value doesn't see to work well either because for some reason, fs.utimes completely misinterprets the number. In that version of node anyway.)

mbargiel · 2019-07-31T02:40:13Z

@manidlou 2b7c0e5 fixes the tests and now both the "fs.copyFile[Sync]" path and "copyFile[Sync]Fallback" path are tested.

In fb89a29, I simplified the implementation following your last idea, but I had to keep some of the complexity you didn't like, because of the catch-22 between fs.utimes truncating millis and utils/utimes.utimesMillis failing if the dest file path is not writable.

mbargiel · 2019-08-08T18:24:15Z

Apparently atime does not have the expected value in a bunch of tests... Looking into it.

mbargiel · 2019-09-09T17:34:25Z

I don't understand why these failures are happening. I cannot reproduce them locally, on macOS (10.14), not even with the same node/npm/nvs versions. All these tests pass. The Linux builder also passes.

The failing builds are using macOS 10.13.3, but passing ones also seem to be using 10.13.3. Maybe a difference between HFS+ and APFS? Could it be there is a race condition between nodejs and the OS APIs, e.g. the source atime is only modified from reading it after the copy is done rather than immediately?

(Edited: the build machines are 10.13.3 only, so it should only be using APFS.)

mbargiel · 2019-09-09T19:33:52Z

I locally tested using a HFS+ partition, no luck. This simply isn't happening on macOS 10.14.6.

mbargiel · 2019-09-09T20:08:05Z

@manidlou f928144 fixes the test failures that I can't quite explain. (Or rather, I can't explain why they were not failures in some builds, considering the Node documentation.) Unfortunately it does so by reviving the "re-read the source file stats" but at least in a way that should be much easier to follow thanks to the suggestions you made in review.

mbargiel · 2019-09-09T20:39:23Z

Note: in #633 (comment), you suggested not setting the mode after the initial copy so we wouldn't need to check it/make writable before updating the timestamps. Unfortunately, the only way I can think of implementing this is to not call fs.copyFile, because it has no option to avoid copying the mode, but instead use what is basically the copyFileFallback function. That is: open the streams, copy the file content, set the timestamps, set the mode, close the streams.

mbargiel · 2019-09-10T11:57:24Z

... I don't get it. I locally see the coverage of the copyFileFallback paths - why doesn't coveralls see it?

EDIT ... because it's running on the commit resulting from merging master, where graceful-fs was re-enabled! Makes sense now. Fixing.

mbargiel · 2019-09-10T17:24:49Z

(Yes, I'll squash it all when it's approved.)

RyanZim · 2020-01-30T15:41:01Z

@manidlou what's the status here?

lib/copy-sync/copy-sync.js

lib/copy/copy.js

manidlou

LGTM! Thank you @mbargiel!

mbargiel · 2020-02-04T15:36:03Z

Squashed into 3121749c051718887fc66052eeb55f19f2eb309a. I also reformulated the commit message (but I guess you're free to set whatever merge commit message 😄 )

Glad to have been of any help!

RyanZim

Actual code looks OK to me; however, please remove the fallback test files. We will simply test the fallback on the old Node versions on the CI where copyFile doesn't exist.

Besides, the fallback will be getting killed soon, as we upgrade minimum required Node version.

mbargiel · 2020-02-04T17:19:33Z

Sure thing. You might want to explicitly opt out of code coverage for "fallback" paths like these in the future? That caused me to run into "code coverage loss" while working on this PR.

mbargiel · 2020-02-04T18:16:34Z

@RyanZim d09b23b is an amended version that does not include these fallback tests.

mbargiel commented Oct 12, 2018

View reviewed changes

mbargiel force-pushed the fix/copy-preserve-timestamps-atime branch 4 times, most recently from 518036c to ca81e73 Compare October 15, 2018 17:21

mbargiel mentioned this pull request Oct 15, 2018

Tests are failing on mac #571

Closed

mbargiel mentioned this pull request Oct 15, 2018

Update standard libraries and update code accordingly #636

Closed

mbargiel force-pushed the fix/copy-preserve-timestamps-atime branch from ca81e73 to b9edbbe Compare May 14, 2019 20:12

mbargiel commented May 14, 2019

View reviewed changes

manidlou reviewed May 26, 2019

View reviewed changes

manidlou mentioned this pull request Jul 30, 2019

Add heuristics to stats.ino comparison when bigint is not available #694

Merged

mbargiel force-pushed the fix/copy-preserve-timestamps-atime branch from d288d84 to 182e112 Compare September 10, 2019 17:24

RyanZim added the feature-copy label Jan 30, 2020

manidlou reviewed Jan 31, 2020

View reviewed changes

lib/copy-sync/copy-sync.js Outdated Show resolved Hide resolved

lib/copy/copy.js Outdated Show resolved Hide resolved

RyanZim requested a review from manidlou February 3, 2020 18:23

manidlou approved these changes Feb 4, 2020

View reviewed changes

mbargiel force-pushed the fix/copy-preserve-timestamps-atime branch from 75756c4 to 3121749 Compare February 4, 2020 15:34

RyanZim added this to the 9.0.0 milestone Feb 4, 2020

RyanZim requested changes Feb 4, 2020

View reviewed changes

RyanZim mentioned this pull request Feb 4, 2020

Drop old Node support #751

Merged

Update dest atime after copy/copy-sync

d09b23b

mbargiel force-pushed the fix/copy-preserve-timestamps-atime branch from 3121749 to d09b23b Compare February 4, 2020 18:15

RyanZim approved these changes Feb 4, 2020

View reviewed changes

RyanZim merged commit 3dac360 into jprichardson:master Feb 4, 2020

manidlou mentioned this pull request Feb 6, 2020

copySync(src, dst, {preserveTimestamps: true}) fails with EACCES when copying a directory contains read-only file on Linux #629

Closed

dependabot bot mentioned this pull request Mar 15, 2021

chore(deps): bump fs-extra from 8.1.0 to 9.1.0 dhis2/app-platform#536

Closed

mfranzke mentioned this pull request Jun 24, 2021

refactor: updated even further dependencies pattern-lab/patternlab-node#1320

Merged

78 tasks

emilyrohrbough mentioned this pull request Feb 7, 2022

chore(deps): update dependency fs-extra to v9 🌟 cypress-io/cypress#19939

Merged

1 task

Update dest atime after copy/copy-sync #633

Update dest atime after copy/copy-sync #633

Conversation

mbargiel commented Oct 12, 2018

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

mbargiel commented Oct 15, 2018 • edited Loading

mbargiel commented May 14, 2019

coveralls commented May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanZim commented May 14, 2019

mbargiel commented May 14, 2019

Choose a reason for hiding this comment

mbargiel May 26, 2019 • edited Loading

Choose a reason for hiding this comment

mbargiel May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargiel May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brodycj commented May 31, 2019

mbargiel commented Jun 1, 2019

mbargiel commented Jul 27, 2019

manidlou commented Jul 30, 2019 • edited Loading

mbargiel commented Jul 30, 2019

mbargiel commented Jul 30, 2019 • edited Loading

mbargiel commented Jul 31, 2019

mbargiel commented Aug 8, 2019

mbargiel commented Sep 9, 2019 • edited Loading

mbargiel commented Sep 9, 2019

mbargiel commented Sep 9, 2019

mbargiel commented Sep 9, 2019

mbargiel commented Sep 10, 2019 • edited Loading

mbargiel commented Sep 10, 2019

RyanZim commented Jan 30, 2020

manidlou left a comment

Choose a reason for hiding this comment

mbargiel commented Feb 4, 2020

RyanZim left a comment

Choose a reason for hiding this comment

mbargiel commented Feb 4, 2020

mbargiel commented Feb 4, 2020

mbargiel commented Oct 15, 2018 •

edited

Loading

coveralls commented May 14, 2019 •

edited

Loading

mbargiel May 26, 2019 •

edited

Loading

mbargiel May 26, 2019 •

edited

Loading

mbargiel May 26, 2019 •

edited

Loading

manidlou commented Jul 30, 2019 •

edited

Loading

mbargiel commented Jul 30, 2019 •

edited

Loading

mbargiel commented Sep 9, 2019 •

edited

Loading

mbargiel commented Sep 10, 2019 •

edited

Loading