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

Use r+ with truncation when saving existing files #31733

Merged
merged 7 commits into from
Aug 15, 2017
Merged

Use r+ with truncation when saving existing files #31733

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc8b380
Use r+ with truncation when saving existing files
seishun Jul 30, 2017
8dbd3e0
use r+ only if EPERM is caught
seishun Aug 11, 2017
5517b03
add check for Windows
seishun Aug 12, 2017
e27c8ca
move writing to a separate method
seishun Aug 12, 2017
bdbfeac
nits
seishun Aug 13, 2017
c7d4931
remove string overload for options
seishun Aug 13, 2017
ab03fee
remove encoding property from writeFileAndFlush
seishun Aug 13, 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
12 changes: 8 additions & 4 deletions src/vs/base/node/pfs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ export function touch(path: string): TPromise<void> {
return nfcall(fs.utimes, path, now, now);
}

export function truncate(path: string, len: number): TPromise<void> {
return nfcall(fs.truncate, path, len);
}

export function readFile(path: string): TPromise<Buffer>;
export function readFile(path: string, encoding: string): TPromise<string>;
export function readFile(path: string, encoding?: string): TPromise<Buffer | string> {
Expand All @@ -120,12 +124,12 @@ export function readFile(path: string, encoding?: string): TPromise<Buffer | str
// Therefor we use a Queue on the path that is given to us to sequentialize calls to the same path properly.
const writeFilePathQueue: { [path: string]: Queue<void> } = Object.create(null);

export function writeFile(path: string, data: string, encoding?: string): TPromise<void>;
export function writeFile(path: string, data: NodeBuffer, encoding?: string): TPromise<void>;
export function writeFile(path: string, data: any, encoding: string = 'utf8'): TPromise<void> {
export function writeFile(path: string, data: string, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can safely remove the encoding option because it was always UTF-8 being used. The reason why an explicit encoding field is dangerous is because not all encodings the VS Code supports are supported by node (that is why we use a third party library for encoding conversion). Let's remove this option and use UTF-8 always in the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the string option or the encoding property?

Copy link
Member

Choose a reason for hiding this comment

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

@seishun the encoding property within the options bag. I think it is good that you removed the previous string property and replaced with a more explicit option bag 👍

export function writeFile(path: string, data: NodeBuffer, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise<void>;
export function writeFile(path: string, data: any, options?: string | { encoding?: string; mode?: number; flag?: string; }): TPromise<void> {
let queueKey = toQueueKey(path);

return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, encoding));
return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, options));
Copy link
Member

Choose a reason for hiding this comment

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

Given this change, can we update extfs.writeFileAndFlush to no longer check for the options bag being a string (in https://github.com/Microsoft/vscode/blob/master/src/vs/base/node/extfs.ts#L342)

}

function toQueueKey(path: string): string {
Expand Down
28 changes: 28 additions & 0 deletions src/vs/workbench/services/files/node/fileService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,34 @@ export class FileService implements IFileService {

// 5.) resolve
return this.resolve(resource);
}, error => {
// Can't use 'w' for hidden files, so truncate and use 'r+' if the file exists
Copy link
Member

Choose a reason for hiding this comment

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

Suggest to reference the issue here in the comment: #931

if (!exists || error.code !== 'EPERM') {
Copy link
Member

Choose a reason for hiding this comment

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

@seishun should we add a check for windows here? as far as I know, hidden files (and this issue) only manifest on windows

throw error;
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use TPromise.wrapError(error) to be consistent with other places in the file

}

// 5.) truncate
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to add a little bit more comment here why the trick of truncate and r+ mode actually works for hidden files. Currently it is hard to understand what is going on in the error case.

return pfs.truncate(absolutePath, 0).then(() => {
let writeFilePromise: TPromise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

@seishun I still suggest to extract those lines which are now duplicate to a separate method that can be called then once from the normal path and once from the error path. that should make the change much smaller and easier to understand 👍


// 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);
});
});
});
});
});
Expand Down