Use r+ with truncation when saving existing files #31733

seishun · 2017-07-30T19:35:55Z

Fixes #931

Here's how things got this way:

Windows API requires the FILE_ATTRIBUTE_HIDDEN flag when calling CreateFile on a hidden file if CREATE_ALWAYS is specified.
Microsoft C run-time library never uses this flag, consequently fopen in w mode fails on hidden files.
libuv follows MS CRT's behavior on Windows and fails with UV_EPERM when uv_fs_open is called with O_WRONLY|O_CREAT|O_TRUNC (aka "w") on a hidden file.
Node.js uses uv_fs_open to implement fs.open and thus fails with EPERM when trying to open a hidden file with flags='w'.
Visual Studio Code opens the file with flags='w' when saving a file, which fails on hidden files. We've gone full circle.

This PR uses r+ when opening existing files and truncates them. We can't do it just for hidden files since Node.js doesn't provide an API to query the hiddenness of a file. We also can't do it for all files since r+ fails if the file doesn't exist.

msftclas · 2017-07-30T19:36:00Z

@seishun,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

msftclas · 2017-07-30T19:42:08Z

@seishun, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

bpasero · 2017-08-11T08:17:57Z

@seishun good start, thanks for looking into this! Instead of running this code everytime we save a file, I would feel more comfortable to catch the EPERM situation and only in that case do your trick. How does that sound?

seishun · 2017-08-11T13:24:46Z

That's fine, but it will add two indentation levels rather than one. I've submitted #32327 to get rid of the callback hell. If it's merged, then it will no longer be a problem.

bpasero · 2017-08-11T13:29:25Z

@seishun hm I see, I added a comment to that PR because changes like that are on the edge of being controversal (for various reasons: should all code use async/await or not, does the author prefer async/await over promises, etc.).

Can we get my suggestion in without forcing the discussion around async/await in the file service?

seishun · 2017-08-11T13:44:00Z

Sure, no problem. I had already converted it as an experiment so I figured that I might as well submit a PR.

An alternative approach would be a chain of then callbacks. It would also avoid multi-level nesting, but it would require declaring some variables (e.g. exists) at the top of the function. Would that work, or do you prefer keeping the current style?

bpasero · 2017-08-11T13:51:25Z

@seishun yeah that is totally fine.

As for async/await, it just seems weird to have a small part of a file use this style and the rest the other style. And it goes beyond that, we need to agree in general where we adopt async/await, it should be consistent imho.

'w' doesn't work with hidden files. Fixes: #931

seishun · 2017-08-11T17:41:17Z

Done. I decided to keep the existing style because assigning function arguments to variables in outer scope doesn't look pretty.

bpasero

@seishun thanks, some feedback provided

bpasero · 2017-08-12T06:50:47Z

src/vs/workbench/services/files/node/fileService.ts

@@ -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
+						if (!exists || error.code !== 'EPERM') {


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

bpasero · 2017-08-12T06:51:25Z

src/vs/workbench/services/files/node/fileService.ts

+
+						// 5.) truncate
+						return pfs.truncate(absolutePath, 0).then(() => {
+							let writeFilePromise: TPromise<void>;


@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 👍

seishun · 2017-08-12T11:42:48Z

@bpasero PTAL

bpasero

Thanks, much better. Only minor requests now.

bpasero · 2017-08-13T07:45:18Z

src/vs/base/node/pfs.ts

 	let queueKey = toQueueKey(path);

-	return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, encoding));
+	return ensureWriteFileQueue(queueKey).queue(() => nfcall(extfs.writeFileAndFlush, path, data, options));


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)

bpasero · 2017-08-13T07:46:12Z

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<IFileStat> {


Some suggestions:

make the method private

rename it to doSetContentsAndResolve

move it below the method updateContent

remove the encoding from the options, it is not being used from the caller and it is also dangerous because in order to support all encodings, we need to use the encoding.encode method instead

bpasero · 2017-08-13T07:46:49Z

src/vs/workbench/services/files/node/fileService.ts

+
+		// Otherwise use encoding lib
+		else {
+			const encoded = encoding.encode(value, encodingToWrite, { addBOM: addBom });


This can actually be simplified to { addBOM } as last argument to encode

Can't, addBOM vs addBom. Or do you propose renaming the parameter?

@seishun oh yeah I see, then maybe just rename the parameter in this method so that you can use the shorthand.

bpasero · 2017-08-13T07:57:07Z

src/vs/base/node/pfs.ts

-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>;


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.

Do you mean the string option or the encoding property?

@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 👍

bpasero · 2017-08-13T07:58:12Z

src/vs/workbench/services/files/node/fileService.ts

-					return writeFilePromise.then(() => {
+					// 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


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

bpasero · 2017-08-13T07:59:09Z

src/vs/workbench/services/files/node/fileService.ts

+					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;


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

bpasero · 2017-08-13T08:00:47Z

src/vs/workbench/services/files/node/fileService.ts


-						// 5.) resolve
-						return this.resolve(resource);
+						// 5.) truncate


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.

seishun · 2017-08-13T09:16:18Z

Nits applied.

seishun · 2017-08-13T12:18:20Z

Also removed the string overload (?) for the options parameter in writeFile since it doesn't work anymore.

bpasero · 2017-08-13T14:35:01Z

@seishun almost good, the only complaint I have now is that extfs.writeFile still has an encoding option which imho we should not expose but just set to UTF-8. E.g. where we access options.encoding in that method, just use utf8 directly and do not expose the option to set it.

seishun · 2017-08-13T14:43:33Z

I removed the argument since it defaults to 'utf8' anyway.

bpasero · 2017-08-15T06:40:36Z

@seishun thanks, merging 👍

msftclas added the cla-required label Jul 30, 2017

msftclas added cla-signed and removed cla-required labels Jul 30, 2017

seishun mentioned this pull request Jul 31, 2017

Can't open hidden files in 'w' mode on Windows - document, fix, or do nothing? nodejs/node#14553

Closed

rebornix assigned bpasero Jul 31, 2017

bpasero added this to the On Deck milestone Aug 11, 2017

seishun added 2 commits August 11, 2017 20:06

Use r+ with truncation when saving existing files

cc8b380

'w' doesn't work with hidden files. Fixes: #931

use r+ only if EPERM is caught

8dbd3e0

bpasero requested changes Aug 12, 2017

View reviewed changes

seishun added 2 commits August 12, 2017 12:42

add check for Windows

5517b03

move writing to a separate method

e27c8ca

bpasero requested changes Aug 13, 2017

View reviewed changes

nits

bdbfeac

bpasero modified the milestones: August 2017, On Deck Aug 13, 2017

remove string overload for options

c7d4931

remove encoding property from writeFileAndFlush

ab03fee

bpasero merged commit cc9d8fc into microsoft:master Aug 15, 2017

seishun deleted the hidden branch September 8, 2017 19:13

seishun mentioned this pull request Feb 3, 2018

Alternate data streams are removed by saving #6363

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use r+ with truncation when saving existing files #31733

Use r+ with truncation when saving existing files #31733

seishun commented Jul 30, 2017 •

edited by bpasero

Loading

msftclas commented Jul 30, 2017

msftclas commented Jul 30, 2017

bpasero commented Aug 11, 2017 •

edited

Loading

seishun commented Aug 11, 2017

bpasero commented Aug 11, 2017 •

edited

Loading

seishun commented Aug 11, 2017 •

edited

Loading

bpasero commented Aug 11, 2017

seishun commented Aug 11, 2017

bpasero left a comment

bpasero Aug 12, 2017

bpasero Aug 12, 2017

seishun commented Aug 12, 2017

bpasero left a comment

bpasero Aug 13, 2017

bpasero Aug 13, 2017

bpasero Aug 13, 2017

seishun Aug 13, 2017

bpasero Aug 13, 2017

bpasero Aug 13, 2017

seishun Aug 13, 2017

bpasero Aug 13, 2017

bpasero Aug 13, 2017

bpasero Aug 13, 2017

bpasero Aug 13, 2017

seishun commented Aug 13, 2017

seishun commented Aug 13, 2017

bpasero commented Aug 13, 2017

seishun commented Aug 13, 2017

bpasero commented Aug 15, 2017

Use r+ with truncation when saving existing files #31733

Use r+ with truncation when saving existing files #31733

Conversation

seishun commented Jul 30, 2017 • edited by bpasero Loading

msftclas commented Jul 30, 2017

msftclas commented Jul 30, 2017

bpasero commented Aug 11, 2017 • edited Loading

seishun commented Aug 11, 2017

bpasero commented Aug 11, 2017 • edited Loading

seishun commented Aug 11, 2017 • edited Loading

bpasero commented Aug 11, 2017

seishun commented Aug 11, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun commented Aug 12, 2017

bpasero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun commented Aug 13, 2017

seishun commented Aug 13, 2017

bpasero commented Aug 13, 2017

seishun commented Aug 13, 2017

bpasero commented Aug 15, 2017

seishun commented Jul 30, 2017 •

edited by bpasero

Loading

bpasero commented Aug 11, 2017 •

edited

Loading

bpasero commented Aug 11, 2017 •

edited

Loading

seishun commented Aug 11, 2017 •

edited

Loading