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.

Sign up for GitHub

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

Update: use fs.copyFile when available #4486

Merged
merged 8 commits into from
Sep 18, 2017
Merged

Update: use fs.copyFile when available #4486

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
110d43a
Update: use fs.copyFile when available
BYK Sep 16, 2017
c7c0072
Don't wait on writePromise
BYK Sep 16, 2017
c38b00f
More readable code
BYK Sep 17, 2017
58f6438
Fix bug in Node < 7
BYK Sep 17, 2017
a42adbd
Increase copy concurrency and add comment about where it came from
BYK Sep 17, 2017
b84289a
Merge branch 'master' into copyFile
BYK Sep 17, 2017
06ce7f5
Use better function flow type
BYK Sep 18, 2017
3a57109
Fix lint
BYK Sep 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ git:
language: node_js

node_js:
- "8"
- "6"
- "4"

Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
environment:
matrix:
- node_version: "8"
- node_version: "6"
- node_version: "4"

Expand Down
2 changes: 1 addition & 1 deletion circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ general:

machine:
node:
version: 6
version: 8

dependencies:
cache_directories:
Expand Down
154 changes: 88 additions & 66 deletions src/util/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if 128 will essentially lock up the system on slower hard drives (particularly if the user is multitasking)

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't since this is still single threaded and we leave the scheduling job to the OS and Node as far as I understand. The reason for having a much lower limit for the fallback is mostly old versions of Node having buffering issues with pipe sometimes and possible memory pressure after #3539 since it now reads the whole file contents into memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this becomes a problem we can always make this a configurable variable.


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,
);
Expand All @@ -61,7 +90,6 @@ export type CopyQueueItem = {
type CopyQueue = Array<CopyQueueItem>;

type CopyFileAction = {
type: 'file',
src: string,
dest: string,
atime: number,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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,
});
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -326,8 +358,7 @@ async function buildActionsForCopy(
}
} else if (srcStat.isFile()) {
onFresh();
actions.push({
type: 'file',
actions.file.push({
src,
dest,
atime: srcStat.atime,
Expand All @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -510,8 +542,7 @@ async function buildActionsForHardlink(
}
} else if (srcStat.isFile()) {
onFresh();
actions.push({
type: 'link',
actions.link.push({
src,
dest,
removeDest: destExists,
Expand All @@ -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: Function): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Use () => void instead of Function for the type of cleanup.

try {
await unlink(data.dest);
await copyFile(data.src, data.dest, 0, data);
} finally {
cleanup();
}
};

export async function copyBulk(
queue: CopyQueue,
reporter: Reporter,
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

You can't replace the while by a if, otherwise multiple waiting instances will all get released all at once. The while ensures that a single one will ever be able to cross the condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this but here's my reasoning:

  • Event loop is single-threaded
  • If I do await promise the rest of the code is essentially gets into promise.then()
  • Since all code after this point is synchronous and since the event loop is single threaded, there can't be a new entry in the map between when the promise get's released and we add a new promise there.

Makes sense, or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Even if the event loop is single threaded, because all iterations will wait on the same promise, all the promises will be released at once (but sequentially) because they won't re-check to make sure they are the first promise to wake up.

That being said, I just saw that this call is inside a promise.queue call, so maybe this utility will already take care of this (but then why await at all?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see! Yes, you're right. For the promise queue part, I don't know why we simply don't return the existing promise. Doesn't make much sense to me since right now we are essentially writing the same file more than 1 time once the promsie is released. I'll change the code accordingly.

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

Choose a reason for hiding this comment

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

I read: cpojer. lol

},
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));
Expand All @@ -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,
Expand All @@ -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));
Expand Down Expand Up @@ -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> {
Expand Down