From cc8b380e8d9eb74fe780b2a78490e0c40b68b7c2 Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sun, 30 Jul 2017 21:36:26 +0300 Subject: [PATCH 1/7] Use r+ with truncation when saving existing files 'w' doesn't work with hidden files. Fixes: https://github.com/Microsoft/vscode/issues/931 --- src/vs/base/node/pfs.ts | 12 +++-- .../services/files/node/fileService.ts | 46 +++++++++++++------ 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/vs/base/node/pfs.ts b/src/vs/base/node/pfs.ts index 453e19a949377..4a1a1c1c6ca31 100644 --- a/src/vs/base/node/pfs.ts +++ b/src/vs/base/node/pfs.ts @@ -109,6 +109,10 @@ export function touch(path: string): TPromise { return nfcall(fs.utimes, path, now, now); } +export function truncate(path: string, len: number): TPromise { + return nfcall(fs.truncate, path, len); +} + export function readFile(path: string): TPromise; export function readFile(path: string, encoding: string): TPromise; export function readFile(path: string, encoding?: string): TPromise { @@ -120,12 +124,12 @@ export function readFile(path: string, encoding?: string): TPromise } = Object.create(null); -export function writeFile(path: string, data: string, encoding?: string): TPromise; -export function writeFile(path: string, data: NodeBuffer, encoding?: string): TPromise; -export function writeFile(path: string, data: any, encoding: string = 'utf8'): TPromise { +export function writeFile(path: string, data: string, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: NodeBuffer, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: any, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise { let queueKey = toQueueKey(path); - return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, encoding)); + return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, options)); } function toQueueKey(path: string): string { diff --git a/src/vs/workbench/services/files/node/fileService.ts b/src/vs/workbench/services/files/node/fileService.ts index db42fdd616a45..88c8cd7ed4b9c 100644 --- a/src/vs/workbench/services/files/node/fileService.ts +++ b/src/vs/workbench/services/files/node/fileService.ts @@ -348,24 +348,44 @@ export class FileService implements IFileService { // 3.) check to add UTF BOM return addBomPromise.then(addBom => { - let writeFilePromise: TPromise; + let truncatePromise: TPromise; - // Write fast if we do UTF 8 without BOM - if (!addBom && encodingToWrite === encoding.UTF8) { - writeFilePromise = pfs.writeFile(absolutePath, value, encoding.UTF8); + // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists + if (exists) { + truncatePromise = pfs.truncate(absolutePath, 0); + } else { + truncatePromise = TPromise.as(null); } - // Otherwise use encoding lib - else { - const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); - writeFilePromise = pfs.writeFile(absolutePath, encoded); - } + // 4.) check to truncate + return truncatePromise.then(() => { + let writeFilePromise: TPromise; + + // Write fast if we do UTF 8 without BOM + if (!addBom && encodingToWrite === encoding.UTF8) { + if (exists) { + writeFilePromise = pfs.writeFile(absolutePath, value, { encoding: encoding.UTF8, mode: 0o666, flag: 'r+' }); + } else { + writeFilePromise = pfs.writeFile(absolutePath, value, encoding.UTF8); + } + } - // 4.) set contents - return writeFilePromise.then(() => { + // Otherwise use encoding lib + else { + const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); + if (exists) { + writeFilePromise = pfs.writeFile(absolutePath, encoded, { mode: 0o666, flag: 'r+' }); + } else { + writeFilePromise = pfs.writeFile(absolutePath, encoded); + } + } + + // 5.) set contents + return writeFilePromise.then(() => { - // 5.) resolve - return this.resolve(resource); + // 6.) resolve + return this.resolve(resource); + }); }); }); }); From 8dbd3e0b7a9236094b7bcf94042d70b555955b3f Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Fri, 11 Aug 2017 20:37:05 +0300 Subject: [PATCH 2/7] use r+ only if EPERM is caught --- .../services/files/node/fileService.ts | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/src/vs/workbench/services/files/node/fileService.ts b/src/vs/workbench/services/files/node/fileService.ts index 88c8cd7ed4b9c..79581de33d7c2 100644 --- a/src/vs/workbench/services/files/node/fileService.ts +++ b/src/vs/workbench/services/files/node/fileService.ts @@ -348,43 +348,51 @@ export class FileService implements IFileService { // 3.) check to add UTF BOM return addBomPromise.then(addBom => { - let truncatePromise: TPromise; + let writeFilePromise: TPromise; - // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists - if (exists) { - truncatePromise = pfs.truncate(absolutePath, 0); - } else { - truncatePromise = TPromise.as(null); + // Write fast if we do UTF 8 without BOM + if (!addBom && encodingToWrite === encoding.UTF8) { + writeFilePromise = pfs.writeFile(absolutePath, value, encoding.UTF8); + } + + // Otherwise use encoding lib + else { + const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); + writeFilePromise = pfs.writeFile(absolutePath, encoded); } - // 4.) check to truncate - return truncatePromise.then(() => { - let writeFilePromise: TPromise; + // 4.) set contents + return writeFilePromise.then(() => { - // Write fast if we do UTF 8 without BOM - if (!addBom && encodingToWrite === encoding.UTF8) { - if (exists) { + // 5.) resolve + return this.resolve(resource); + }, error => { + // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists + if (!exists || error.code !== 'EPERM') { + throw error; + } + + // 5.) truncate + return pfs.truncate(absolutePath, 0).then(() => { + let writeFilePromise: TPromise; + + // Write fast if we do UTF 8 without BOM + if (!addBom && encodingToWrite === encoding.UTF8) { writeFilePromise = pfs.writeFile(absolutePath, value, { encoding: encoding.UTF8, mode: 0o666, flag: 'r+' }); - } else { - writeFilePromise = pfs.writeFile(absolutePath, value, encoding.UTF8); } - } - // Otherwise use encoding lib - else { - const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); - if (exists) { + // Otherwise use encoding lib + else { + const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); writeFilePromise = pfs.writeFile(absolutePath, encoded, { mode: 0o666, flag: 'r+' }); - } else { - writeFilePromise = pfs.writeFile(absolutePath, encoded); } - } - // 5.) set contents - return writeFilePromise.then(() => { + // 6.) set contents + return writeFilePromise.then(() => { - // 6.) resolve - return this.resolve(resource); + // 7.) resolve + return this.resolve(resource); + }); }); }); }); From 5517b039d29d982f484825aa95fa2bdc9519a70f Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sat, 12 Aug 2017 12:42:35 +0300 Subject: [PATCH 3/7] add check for Windows --- src/vs/workbench/services/files/node/fileService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vs/workbench/services/files/node/fileService.ts b/src/vs/workbench/services/files/node/fileService.ts index 79581de33d7c2..3d2888809de7a 100644 --- a/src/vs/workbench/services/files/node/fileService.ts +++ b/src/vs/workbench/services/files/node/fileService.ts @@ -368,7 +368,7 @@ export class FileService implements IFileService { return this.resolve(resource); }, error => { // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists - if (!exists || error.code !== 'EPERM') { + if (!exists || error.code !== 'EPERM' || !isWindows) { throw error; } From e27c8ca514ca436b798c4ab1876df3086094f897 Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sat, 12 Aug 2017 14:38:08 +0300 Subject: [PATCH 4/7] move writing to a separate method --- .../services/files/node/fileService.ts | 65 ++++++++----------- 1 file changed, 27 insertions(+), 38 deletions(-) diff --git a/src/vs/workbench/services/files/node/fileService.ts b/src/vs/workbench/services/files/node/fileService.ts index 3d2888809de7a..53783a88e2b2d 100644 --- a/src/vs/workbench/services/files/node/fileService.ts +++ b/src/vs/workbench/services/files/node/fileService.ts @@ -315,6 +315,29 @@ export class FileService implements IFileService { }); } + public setContentsAndResolve(resource: uri, absolutePath: string, value: string, addBom: boolean, encodingToWrite: string, options: { encoding?: string; mode?: number; flag?: string; }): TPromise { + let writeFilePromise: TPromise; + + // Write fast if we do UTF 8 without BOM + if (!addBom && encodingToWrite === encoding.UTF8) { + options.encoding = encoding.UTF8; + writeFilePromise = pfs.writeFile(absolutePath, value, options); + } + + // Otherwise use encoding lib + else { + const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); + writeFilePromise = pfs.writeFile(absolutePath, encoded, options); + } + + // set contents + return writeFilePromise.then(() => { + + // resolve + return this.resolve(resource); + }); + } + public updateContent(resource: uri, value: string, options: IUpdateContentOptions = Object.create(null)): TPromise { const absolutePath = this.toAbsolutePath(resource); @@ -348,25 +371,8 @@ export class FileService implements IFileService { // 3.) check to add UTF BOM return addBomPromise.then(addBom => { - let writeFilePromise: TPromise; - - // Write fast if we do UTF 8 without BOM - if (!addBom && encodingToWrite === encoding.UTF8) { - writeFilePromise = pfs.writeFile(absolutePath, value, encoding.UTF8); - } - - // Otherwise use encoding lib - else { - const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); - writeFilePromise = pfs.writeFile(absolutePath, encoded); - } - - // 4.) set contents - return writeFilePromise.then(() => { - - // 5.) resolve - return this.resolve(resource); - }, error => { + // 4.) set contents and resolve + return this.setContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'w' }).then(undefined, error => { // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists if (!exists || error.code !== 'EPERM' || !isWindows) { throw error; @@ -374,25 +380,8 @@ export class FileService implements IFileService { // 5.) truncate return pfs.truncate(absolutePath, 0).then(() => { - let writeFilePromise: TPromise; - - // Write fast if we do UTF 8 without BOM - if (!addBom && encodingToWrite === encoding.UTF8) { - writeFilePromise = pfs.writeFile(absolutePath, value, { encoding: encoding.UTF8, mode: 0o666, flag: 'r+' }); - } - - // Otherwise use encoding lib - else { - const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); - writeFilePromise = pfs.writeFile(absolutePath, encoded, { mode: 0o666, flag: 'r+' }); - } - - // 6.) set contents - return writeFilePromise.then(() => { - - // 7.) resolve - return this.resolve(resource); - }); + // 6.) set contents and resolve again + return this.setContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'r+' }); }); }); }); From bdbfeac48be0ba32e7cfaf454a10c21e3a330059 Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sun, 13 Aug 2017 12:15:37 +0300 Subject: [PATCH 5/7] nits --- src/vs/base/node/extfs.ts | 2 - src/vs/base/node/pfs.ts | 6 +-- .../services/files/node/fileService.ts | 54 +++++++++---------- 3 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/vs/base/node/extfs.ts b/src/vs/base/node/extfs.ts index 6b99256462b6a..7b03dddb9ebcb 100644 --- a/src/vs/base/node/extfs.ts +++ b/src/vs/base/node/extfs.ts @@ -339,8 +339,6 @@ export function writeFileAndFlush(path: string, data: string | NodeBuffer, optio if (!options) { options = { encoding: 'utf8', mode: 0o666, flag: 'w' }; - } else if (typeof options === 'string') { - options = { encoding: options, mode: 0o666, flag: 'w' }; } // Open the file with same flags and mode as fs.writeFile() diff --git a/src/vs/base/node/pfs.ts b/src/vs/base/node/pfs.ts index 4a1a1c1c6ca31..37d548ad5cb4d 100644 --- a/src/vs/base/node/pfs.ts +++ b/src/vs/base/node/pfs.ts @@ -124,9 +124,9 @@ export function readFile(path: string, encoding?: string): TPromise } = Object.create(null); -export function writeFile(path: string, data: string, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise; -export function writeFile(path: string, data: NodeBuffer, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise; -export function writeFile(path: string, data: any, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise { +export function writeFile(path: string, data: string, options?: string | { mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: NodeBuffer, options?: string | { mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: any, options?: string | { mode?: number; flag?: string; }): TPromise { let queueKey = toQueueKey(path); return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, options)); diff --git a/src/vs/workbench/services/files/node/fileService.ts b/src/vs/workbench/services/files/node/fileService.ts index 53783a88e2b2d..714435c2af4bd 100644 --- a/src/vs/workbench/services/files/node/fileService.ts +++ b/src/vs/workbench/services/files/node/fileService.ts @@ -315,29 +315,6 @@ export class FileService implements IFileService { }); } - public setContentsAndResolve(resource: uri, absolutePath: string, value: string, addBom: boolean, encodingToWrite: string, options: { encoding?: string; mode?: number; flag?: string; }): TPromise { - let writeFilePromise: TPromise; - - // Write fast if we do UTF 8 without BOM - if (!addBom && encodingToWrite === encoding.UTF8) { - options.encoding = encoding.UTF8; - writeFilePromise = pfs.writeFile(absolutePath, value, options); - } - - // Otherwise use encoding lib - else { - const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom }); - writeFilePromise = pfs.writeFile(absolutePath, encoded, options); - } - - // set contents - return writeFilePromise.then(() => { - - // resolve - return this.resolve(resource); - }); - } - public updateContent(resource: uri, value: string, options: IUpdateContentOptions = Object.create(null)): TPromise { const absolutePath = this.toAbsolutePath(resource); @@ -372,16 +349,17 @@ export class FileService implements IFileService { // 3.) check to add UTF BOM return addBomPromise.then(addBom => { // 4.) set contents and resolve - return this.setContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'w' }).then(undefined, error => { - // Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists + return this.doSetContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'w' }).then(undefined, error => { + // Can't use 'w' for hidden files on Windows (see https://github.com/Microsoft/vscode/issues/931) if (!exists || error.code !== 'EPERM' || !isWindows) { - throw error; + return TPromise.wrapError(error); } + // 'r+' always works for existing files, but we need to truncate manually // 5.) truncate return pfs.truncate(absolutePath, 0).then(() => { // 6.) set contents and resolve again - return this.setContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'r+' }); + return this.doSetContentsAndResolve(resource, absolutePath, value, addBom, encodingToWrite, { mode: 0o666, flag: 'r+' }); }); }); }); @@ -389,6 +367,28 @@ export class FileService implements IFileService { }); } + private doSetContentsAndResolve(resource: uri, absolutePath: string, value: string, addBOM: boolean, encodingToWrite: string, options: { mode?: number; flag?: string; }): TPromise { + let writeFilePromise: TPromise; + + // Write fast if we do UTF 8 without BOM + if (!addBOM && encodingToWrite === encoding.UTF8) { + writeFilePromise = pfs.writeFile(absolutePath, value, options); + } + + // Otherwise use encoding lib + else { + const encoded = encoding.encode(value, encodingToWrite, { addBOM }); + writeFilePromise = pfs.writeFile(absolutePath, encoded, options); + } + + // set contents + return writeFilePromise.then(() => { + + // resolve + return this.resolve(resource); + }); + } + public createFile(resource: uri, content: string = ''): TPromise { // Create file From c7d4931ade7e3b0cb7ef5814df5a37e43fdef7e3 Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sun, 13 Aug 2017 15:16:42 +0300 Subject: [PATCH 6/7] remove string overload for options --- src/vs/base/node/pfs.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vs/base/node/pfs.ts b/src/vs/base/node/pfs.ts index 37d548ad5cb4d..16f8d7509be68 100644 --- a/src/vs/base/node/pfs.ts +++ b/src/vs/base/node/pfs.ts @@ -124,9 +124,9 @@ export function readFile(path: string, encoding?: string): TPromise } = Object.create(null); -export function writeFile(path: string, data: string, options?: string | { mode?: number; flag?: string; }): TPromise; -export function writeFile(path: string, data: NodeBuffer, options?: string | { mode?: number; flag?: string; }): TPromise; -export function writeFile(path: string, data: any, options?: string | { mode?: number; flag?: string; }): TPromise { +export function writeFile(path: string, data: string, options?: { mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: NodeBuffer, options?: { mode?: number; flag?: string; }): TPromise; +export function writeFile(path: string, data: any, options?: { mode?: number; flag?: string; }): TPromise { let queueKey = toQueueKey(path); return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, options)); From ab03feecd516d2557d0b6a61a9ff7ec66cc4d8ff Mon Sep 17 00:00:00 2001 From: Nikolai Vavilov Date: Sun, 13 Aug 2017 17:42:56 +0300 Subject: [PATCH 7/7] remove encoding property from writeFileAndFlush --- src/vs/base/node/extfs.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vs/base/node/extfs.ts b/src/vs/base/node/extfs.ts index 7b03dddb9ebcb..3be832166cb52 100644 --- a/src/vs/base/node/extfs.ts +++ b/src/vs/base/node/extfs.ts @@ -332,13 +332,13 @@ export function mv(source: string, target: string, callback: (error: Error) => v // // See https://github.com/nodejs/node/blob/v5.10.0/lib/fs.js#L1194 let canFlush = true; -export function writeFileAndFlush(path: string, data: string | NodeBuffer, options: { encoding?: string; mode?: number; flag?: string; }, callback: (error: Error) => void): void { +export function writeFileAndFlush(path: string, data: string | NodeBuffer, options: { mode?: number; flag?: string; }, callback: (error: Error) => void): void { if (!canFlush) { return fs.writeFile(path, data, options, callback); } if (!options) { - options = { encoding: 'utf8', mode: 0o666, flag: 'w' }; + options = { mode: 0o666, flag: 'w' }; } // Open the file with same flags and mode as fs.writeFile() @@ -348,7 +348,7 @@ export function writeFileAndFlush(path: string, data: string | NodeBuffer, optio } // It is valid to pass a fd handle to fs.writeFile() and this will keep the handle open! - fs.writeFile(fd, data, options.encoding, (writeError) => { + fs.writeFile(fd, data, (writeError) => { if (writeError) { return fs.close(fd, () => callback(writeError)); // still need to close the handle on error! }