Update: use fs.copyFile when available (#4486)

**Summary** Fixes #4331. Supersedes #3290. Uses the newly added `fs.copyFile` on Node 8.5 hen available and falls back to the old buffer based method otherwise. This patch also refactors the file copy code a bit making it more efficient. Here are the durations on my computer with [this package.json](https://github.com/sciolist/fcopy/blob/c0ded8b82bbaa3f4cdb0a67e951eb3f8b0f97fb7/perf/project/alottafiles/package.json): | master | w/o copyFile | w/ copyFile | | - | - | - | | ~23s | ~19s | ~14s | This is with `yarn.lock` in place and w/o `node_modules`. **Test plan** CI should pass.
yarnpkg · Sep 18, 2017 · 0ee1c55 · 0ee1c55
1 parent 0e16ee9
commit 0ee1c55
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 67 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -10,6 +10,7 @@ git:
 language: node_js
 
 node_js:
+  - "8"
   - "6"
   - "4"
 
@@ -38,10 +39,20 @@ matrix:
       node_js: "4"
     - env: TEST_TYPE="lint"
       node_js: "4"
+    - env: TEST_TYPE="build-dist"
+      node_js: "6"
+    - env: TEST_TYPE="check-lockfile"
+      node_js: "6"
+    - env: TEST_TYPE="lint"
+      node_js: "6"
+
   include:
     - os: osx
       node_js: "6"
       env: TEST_TYPE="test-only"
+    - os: osx
+      node_js: "8"
+      env: TEST_TYPE="test-only"
 
 before_install:
   - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then

diff --git a/appveyor.yml b/appveyor.yml
@@ -1,5 +1,6 @@
 environment:
   matrix:
+    - node_version: "8"
     - node_version: "6"
     - node_version: "4"
 

diff --git a/circle.yml b/circle.yml
@@ -7,7 +7,7 @@ general:
 
 machine:
   node:
-    version: 6
+    version: 8
 
 dependencies:
   cache_directories:

diff --git a/src/util/fs.js b/src/util/fs.js
@@ -40,8 +40,37 @@ export const chmod: (path: string, mode: number | string) => Promise<void> = pro
 export const link: (src: string, dst: string) => Promise<fs.Stats> = promisify(fs.link);
 export const glob: (path: string, options?: Object) => Promise<Array<string>> = promisify(globModule);
 
-const CONCURRENT_QUEUE_ITEMS = 4;
-
+// fs.copyFile uses the native file copying instructions on the system, performing much better
+// than any JS-based solution and consumes fewer resources. Repeated testing to fine tune the
+// concurrency level revealed 128 as the sweet spot on a quad-core, 16 CPU Intel system with SSD.
+const CONCURRENT_QUEUE_ITEMS = fs.copyFile ? 128 : 4;
+
+const open: (path: string, flags: string | number, mode: number) => Promise<number> = promisify(fs.open);
+const close: (fd: number) => Promise<void> = promisify(fs.close);
+const write: (
+  fd: number,
+  buffer: Buffer,
+  offset: ?number,
+  length: ?number,
+  position: ?number,
+) => Promise<void> = promisify(fs.write);
+const futimes: (fd: number, atime: number, mtime: number) => Promise<void> = promisify(fs.futimes);
+const copyFile: (src: string, dest: string, flags: number, data: CopyFileAction) => Promise<void> = fs.copyFile
+  ? // Don't use `promisify` to avoid passing  the last, argument `data`, to the native method
+    (src, dest, flags, data) =>
+      new Promise((resolve, reject) => fs.copyFile(src, dest, flags, err => (err ? reject(err) : resolve(err))))
+  : async (src, dest, flags, data) => {
+      // Use open -> write -> futimes -> close sequence to avoid opening the file twice:
+      // one with writeFile and one with utimes
+      const fd = await open(dest, 'w', data.mode);
+      try {
+        const buffer = await readFileBuffer(src);
+        await write(fd, buffer, 0, buffer.length);
+        await futimes(fd, data.atime, data.mtime);
+      } finally {
+        await close(fd);
+      }
+    };
 const fsSymlink: (target: string, path: string, type?: 'dir' | 'file' | 'junction') => Promise<void> = promisify(
   fs.symlink,
 );
@@ -61,7 +90,6 @@ export type CopyQueueItem = {
 type CopyQueue = Array<CopyQueueItem>;
 
 type CopyFileAction = {
-  type: 'file',
   src: string,
   dest: string,
   atime: number,
@@ -70,19 +98,21 @@ type CopyFileAction = {
 };
 
 type LinkFileAction = {
-  type: 'link',
   src: string,
   dest: string,
   removeDest: boolean,
 };
 
 type CopySymlinkAction = {
-  type: 'symlink',
   dest: string,
   linkname: string,
 };
 
-type CopyActions = Array<CopyFileAction | CopySymlinkAction | LinkFileAction>;
+type CopyActions = {
+  file: Array<CopyFileAction>,
+  symlink: Array<CopySymlinkAction>,
+  link: Array<LinkFileAction>,
+};
 
 type CopyOptions = {
   onProgress: (dest: string) => void,
@@ -154,7 +184,11 @@ async function buildActionsForCopy(
   events.onStart(queue.length);
 
   // start building actions
-  const actions: CopyActions = [];
+  const actions: CopyActions = {
+    file: [],
+    symlink: [],
+    link: [],
+  };
 
   // custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items
   // at a time due to the requirement to push items onto the queue
@@ -180,7 +214,7 @@ async function buildActionsForCopy(
   return actions;
 
   //
-  async function build(data): Promise<void> {
+  async function build(data: CopyQueueItem): Promise<void> {
     const {src, dest, type} = data;
     const onFresh = data.onFresh || noop;
     const onDone = data.onDone || noop;
@@ -196,8 +230,7 @@ async function buildActionsForCopy(
     if (type === 'symlink') {
       await mkdirp(path.dirname(dest));
       onFresh();
-      actions.push({
-        type: 'symlink',
+      actions.symlink.push({
         dest,
         linkname: src,
       });
@@ -288,10 +321,9 @@ async function buildActionsForCopy(
     if (srcStat.isSymbolicLink()) {
       onFresh();
       const linkname = await readlink(src);
-      actions.push({
+      actions.symlink.push({
         dest,
         linkname,
-        type: 'symlink',
       });
       onDone();
     } else if (srcStat.isDirectory()) {
@@ -326,8 +358,7 @@ async function buildActionsForCopy(
       }
     } else if (srcStat.isFile()) {
       onFresh();
-      actions.push({
-        type: 'file',
+      actions.file.push({
         src,
         dest,
         atime: srcStat.atime,
@@ -352,18 +383,20 @@ async function buildActionsForHardlink(
 
   // initialise events
   for (const item of queue) {
-    const onDone = item.onDone;
+    const onDone = item.onDone || noop;
     item.onDone = () => {
       events.onProgress(item.dest);
-      if (onDone) {
-        onDone();
-      }
+      onDone();
     };
   }
   events.onStart(queue.length);
 
   // start building actions
-  const actions: CopyActions = [];
+  const actions: CopyActions = {
+    file: [],
+    symlink: [],
+    link: [],
+  };
 
   // custom concurrency logic as we're always executing stacks of CONCURRENT_QUEUE_ITEMS queue items
   // at a time due to the requirement to push items onto the queue
@@ -389,7 +422,7 @@ async function buildActionsForHardlink(
   return actions;
 
   //
-  async function build(data): Promise<void> {
+  async function build(data: CopyQueueItem): Promise<void> {
     const {src, dest} = data;
     const onFresh = data.onFresh || noop;
     const onDone = data.onDone || noop;
@@ -474,8 +507,7 @@ async function buildActionsForHardlink(
     if (srcStat.isSymbolicLink()) {
       onFresh();
       const linkname = await readlink(src);
-      actions.push({
-        type: 'symlink',
+      actions.symlink.push({
         dest,
         linkname,
       });
@@ -510,8 +542,7 @@ async function buildActionsForHardlink(
       }
     } else if (srcStat.isFile()) {
       onFresh();
-      actions.push({
-        type: 'link',
+      actions.link.push({
         src,
         dest,
         removeDest: destExists,
@@ -527,6 +558,23 @@ export function copy(src: string, dest: string, reporter: Reporter): Promise<voi
   return copyBulk([{src, dest}], reporter);
 }
 
+/**
+ * Unlinks the destination to force a recreation. This is needed on case-insensitive file systems
+ * to force the correct naming when the filename has changed only in charater-casing. (Jest -> jest).
+ * It also calls a cleanup function once it is done.
+ *
+ * `data` contains target file attributes like mode, atime and mtime. Built-in copyFile copies these
+ * automatically but our polyfill needs the do this manually, thus needs the info.
+ */
+const safeCopyFile = async function(data: CopyFileAction, cleanup: () => mixed): Promise<void> {
+  try {
+    await unlink(data.dest);
+    await copyFile(data.src, data.dest, 0, data);
+  } finally {
+    cleanup();
+  }
+};
+
 export async function copyBulk(
   queue: CopyQueue,
   reporter: Reporter,
@@ -547,57 +595,31 @@ export async function copyBulk(
   };
 
   const actions: CopyActions = await buildActionsForCopy(queue, events, events.possibleExtraneous, reporter);
-  events.onStart(actions.length);
+  events.onStart(actions.file.length + actions.symlink.length + actions.link.length);
 
-  const fileActions: Array<CopyFileAction> = (actions.filter(action => action.type === 'file'): any);
+  const fileActions: Array<CopyFileAction> = actions.file;
 
-  const currentlyWriting: {[dest: string]: Promise<void>} = {};
+  const currentlyWriting: Map<string, Promise<void>> = new Map();
 
   await promise.queue(
     fileActions,
-    async (data): Promise<void> => {
-      let writePromise: Promise<void>;
-      while ((writePromise = currentlyWriting[data.dest])) {
-        await writePromise;
+    (data: CopyFileAction): Promise<void> => {
+      const writePromise = currentlyWriting.get(data.dest);
+      if (writePromise) {
+        return writePromise;
       }
 
-      const cleanup = () => delete currentlyWriting[data.dest];
       reporter.verbose(reporter.lang('verboseFileCopy', data.src, data.dest));
-      return (currentlyWriting[data.dest] = readFileBuffer(data.src)
-        .then(async d => {
-          // we need to do this because of case-insensitive filesystems, which wouldn't properly
-          // change the file name in case of a file being renamed
-          await unlink(data.dest);
-
-          return writeFile(data.dest, d, {mode: data.mode});
-        })
-        .then(() => {
-          return new Promise((resolve, reject) => {
-            fs.utimes(data.dest, data.atime, data.mtime, err => {
-              if (err) {
-                reject(err);
-              } else {
-                resolve();
-              }
-            });
-          });
-        })
-        .then(
-          () => {
-            events.onProgress(data.dest);
-            cleanup();
-          },
-          err => {
-            cleanup();
-            throw err;
-          },
-        ));
+      const copier = safeCopyFile(data, () => currentlyWriting.delete(data.dest));
+      currentlyWriting.set(data.dest, copier);
+      events.onProgress(data.dest);
+      return copier;
     },
     CONCURRENT_QUEUE_ITEMS,
   );
 
   // we need to copy symlinks last as they could reference files we were copying
-  const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
+  const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
   await promise.queue(symlinkActions, (data): Promise<void> => {
     const linkname = path.resolve(path.dirname(data.dest), data.linkname);
     reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
@@ -624,9 +646,9 @@ export async function hardlinkBulk(
   };
 
   const actions: CopyActions = await buildActionsForHardlink(queue, events, events.possibleExtraneous, reporter);
-  events.onStart(actions.length);
+  events.onStart(actions.file.length + actions.symlink.length + actions.link.length);
 
-  const fileActions: Array<LinkFileAction> = (actions.filter(action => action.type === 'link'): any);
+  const fileActions: Array<LinkFileAction> = actions.link;
 
   await promise.queue(
     fileActions,
@@ -641,7 +663,7 @@ export async function hardlinkBulk(
   );
 
   // we need to copy symlinks last as they could reference files we were copying
-  const symlinkActions: Array<CopySymlinkAction> = (actions.filter(action => action.type === 'symlink'): any);
+  const symlinkActions: Array<CopySymlinkAction> = actions.symlink;
   await promise.queue(symlinkActions, (data): Promise<void> => {
     const linkname = path.resolve(path.dirname(data.dest), data.linkname);
     reporter.verbose(reporter.lang('verboseFileSymlink', data.dest, linkname));
@@ -836,7 +858,7 @@ export async function writeFilePreservingEol(path: string, data: string): Promis
   if (eol !== '\n') {
     data = data.replace(/\n/g, eol);
   }
-  await promisify(fs.writeFile)(path, data);
+  await writeFile(path, data);
 }
 
 export async function hardlinksWork(dir: string): Promise<boolean> {