-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Update: use fs.copyFile when available #4486
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
110d43a
Update: use fs.copyFile when available
BYK c7c0072
Don't wait on writePromise
BYK c38b00f
More readable code
BYK 58f6438
Fix bug in Node < 7
BYK a42adbd
Increase copy concurrency and add comment about where it came from
BYK b84289a
Merge branch 'master' into copyFile
BYK 06ce7f5
Use better function flow type
BYK 3a57109
Fix lint
BYK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ general: | |
|
||
machine: | ||
node: | ||
version: 6 | ||
version: 8 | ||
|
||
dependencies: | ||
cache_directories: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
|
@@ -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> { | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.