Use r+ with truncation when saving existing files by seishun · Pull Request #31733 · microsoft/vscode

Nikolai Vavilov (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.

Microsoft Contribution License Agreements (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

Microsoft Contribution License Agreements (msftclas) · 2017-07-30T19:42:08Z

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

Thanks, Microsoft Pull Request Bot

Benjamin Pasero (bpasero) · 2017-08-11T08:17:57Z

Nikolai Vavilov (@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?

Nikolai Vavilov (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.

Benjamin Pasero (bpasero) · 2017-08-11T13:29:25Z

Nikolai Vavilov (@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?

Nikolai Vavilov (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?

Benjamin Pasero (bpasero) · 2017-08-11T13:51:25Z

Nikolai Vavilov (@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

Nikolai Vavilov (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.

Benjamin Pasero (bpasero)

Nikolai Vavilov (@seishun) thanks, some feedback provided

Benjamin Pasero (bpasero) · 2017-08-12T06:50:47Z

 						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') {


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

Benjamin Pasero (bpasero) · 2017-08-12T06:51:25Z

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


Nikolai Vavilov (@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 👍

Nikolai Vavilov (seishun) · 2017-08-12T11:42:48Z

Benjamin Pasero (@bpasero) PTAL

Benjamin Pasero (bpasero)

Thanks, much better. Only minor requests now.

Benjamin Pasero (bpasero) · 2017-08-13T07:45:18Z

 	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)

Benjamin Pasero (bpasero) · 2017-08-13T07:46:12Z

 		});
 	}

+	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

Benjamin Pasero (bpasero) · 2017-08-13T07:46:49Z

+
+		// 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?

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

Benjamin Pasero (bpasero) · 2017-08-13T07:57:07Z

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

Nikolai Vavilov (@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 👍

Benjamin Pasero (bpasero) · 2017-08-13T07:58:12Z

-					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

Benjamin Pasero (bpasero) · 2017-08-13T07:59:09Z

+					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

Benjamin Pasero (bpasero) · 2017-08-13T08:00:47Z


-						// 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.

Nikolai Vavilov (seishun) · 2017-08-13T09:16:18Z

Nits applied.

Nikolai Vavilov (seishun) · 2017-08-13T12:18:20Z

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

Benjamin Pasero (bpasero) · 2017-08-13T14:35:01Z

Nikolai Vavilov (@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.

Nikolai Vavilov (seishun) · 2017-08-13T14:43:33Z

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

Benjamin Pasero (bpasero) · 2017-08-15T06:40:36Z

Nikolai Vavilov (@seishun) thanks, merging 👍

Microsoft Contribution License Agreements (msftclas) added the cla-required label Jul 30, 2017

Microsoft Contribution License Agreements (msftclas) added cla-signed and removed cla-required labels Jul 30, 2017

Nikolai Vavilov (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

Peng Lyu (rebornix) assigned Benjamin Pasero (bpasero) Jul 31, 2017

Benjamin Pasero (bpasero) added this to the On Deck milestone Aug 11, 2017

Nikolai Vavilov (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

Benjamin Pasero (bpasero) suggested changes Aug 12, 2017

View reviewed changes

Nikolai Vavilov (seishun) added 2 commits August 12, 2017 12:42

add check for Windows

5517b03

move writing to a separate method

e27c8ca

Benjamin Pasero (bpasero) suggested changes Aug 13, 2017

View reviewed changes

nits

bdbfeac

Benjamin Pasero (bpasero) modified the milestones: August 2017, On Deck Aug 13, 2017

remove string overload for options

c7d4931

remove encoding property from writeFileAndFlush

ab03fee

Benjamin Pasero (bpasero) merged commit cc9d8fc into microsoft:master Aug 15, 2017

Nikolai Vavilov (seishun) deleted the hidden branch September 8, 2017 19:13

Nikolai Vavilov (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

Conversation

Nikolai Vavilov (seishun) commented Jul 30, 2017 • edited by bpasero Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Jul 30, 2017

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Jul 30, 2017

Uh oh!

Benjamin Pasero (bpasero) commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nikolai Vavilov (seishun) commented Aug 11, 2017

Uh oh!

Benjamin Pasero (bpasero) commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nikolai Vavilov (seishun) commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Benjamin Pasero (bpasero) commented Aug 11, 2017

Uh oh!

Nikolai Vavilov (seishun) commented Aug 11, 2017

Uh oh!

Benjamin Pasero (bpasero) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nikolai Vavilov (seishun) commented Aug 12, 2017

Uh oh!

Benjamin Pasero (bpasero) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nikolai Vavilov (seishun) commented Aug 13, 2017

Uh oh!

Nikolai Vavilov (seishun) commented Aug 13, 2017

Uh oh!

Benjamin Pasero (bpasero) commented Aug 13, 2017

Uh oh!

Nikolai Vavilov (seishun) commented Aug 13, 2017

Uh oh!

Benjamin Pasero (bpasero) commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nikolai Vavilov (seishun) commented Jul 30, 2017 •

edited by bpasero

Loading

Benjamin Pasero (bpasero) commented Aug 11, 2017 •

edited

Loading

Benjamin Pasero (bpasero) commented Aug 11, 2017 •

edited

Loading

Nikolai Vavilov (seishun) commented Aug 11, 2017 •

edited

Loading