Fixes race conditions of writing and reading settings from multiple windows

[sandy081]

This PR fixes #125970

• move writing user configuration to main process
• safe guard read from writing in parallel from other windows

[bpasero]

I am not a big fan, especially since this seems to be restoring the loop for iterating over the file if the contents are empty due to truncate.

Given all this, shouldn't the file service provide a atomic write option and then everything can stay the same? And the implementation is a write to tmp file and then move to target?

[sandy081]

I would be happy to use If the file service supports atomic write across windows. I thought about writing to tmp and renaming as you suggested, but I think this cannot handle race conditions especially when two windows tries to write settings. I think in this case last operation will replace previous ones that can cause lose of content. I am not sure if this can be handled. The current approach of single (main or shared in future) process handling writes will make sure there will be no race conditions. When we move to sandbox solution where I am guessing fs operations are moved to shared process then I can remove these loops and use atomic reads while reading settings from workbench.

[sandy081]

I think what we needed is here not atomic write but global atomic read as writes are safe because we throw exception (file modified since). It's the read we need to make atomic so that we do not read while writes are happening.

[bpasero]

I think this is an OK intermediate solution until we have the file service in a central place, so please file a follow up issue labeled sandbox to revisit this change. Ideally:

• it wouldn't be the main service doing the writes but some child process / shared process
• it wouldn't require you to loop in a polling interval to detect empty reads

Btw, the dirty write prevention logic ONLY works if you pass in a etag and mtime as part of the writeFile options. There is no internal tracking for this otherwise, so I doubt you benefit from this unless you would actually go through a text file model for writing.

See

vscode/src/vs/platform/files/common/fileService.ts

Lines 408 to 426 in 466dd4e

	// Dirty write prevention: if the file on disk has been changed and does not match our expected
	// mtime and etag, we bail out to prevent dirty writing.
	//
	// First, we check for a mtime that is in the future before we do more checks. The assumption is
	// that only the mtime is an indicator for a file that has changed on disk.
	//
	// Second, if the mtime has advanced, we compare the size of the file on disk with our previous
	// one using the etag() function. Relying only on the mtime check has prooven to produce false
	// positives due to file system weirdness (especially around remote file systems). As such, the
	// check for size is a weaker check because it can return a false negative if the file has changed
	// but to the same length. This is a compromise we take to avoid having to produce checksums of
	// the file content for comparison which would be much slower to compute.
	if (
	options && typeof options.mtime === 'number' && typeof options.etag === 'string' && options.etag !== ETAG_DISABLED &&
	typeof stat.mtime === 'number' && typeof stat.size === 'number' &&
	options.mtime < stat.mtime && options.etag !== etag({ mtime: options.mtime /* not using stat.mtime for a reason, see above */, size: stat.size })
	) {
	throw new FileOperationError(localize('fileModifiedError', "File Modified Since"), FileOperationResult.FILE_MODIFIED_SINCE, options);
	}

[sandy081]

Follow up issue - #126376

Btw, the dirty write prevention logic ONLY works if you pass in a etag and mtime as part of the writeFile options.

You are right. I am taking care of this in IUserConfigurationFileService - that writes user settings - to read and write the file using e-tag and mtime:

vscode/src/vs/platform/configuration/common/userConfigurationFileService.ts

Lines 54 to 76 in 155de9d

	private async doWrite(resource: URI, jsonValue: IJSONValue, formattingOptions: FormattingOptions): Promise<void> {
	this.logService.trace(`${UserConfigurationFileServiceId}#write`, resource.toString(), jsonValue);
	const { value, mtime, etag } = await this.fileService.readFile(resource, { atomic: true });
	let content = value.toString();
	const parseErrors: ParseError[] = [];
	parse(content, parseErrors, { allowTrailingComma: true, allowEmptyContent: true });
	if (parseErrors.length) {
	throw new Error(UserConfigurationErrorCode.ERROR_INVALID_FILE);
	}
	const edit = this.getEdits(jsonValue, content, formattingOptions)[0];
	if (edit) {
	content = content.substring(0, edit.offset) + edit.content + content.substring(edit.offset + edit.length);
	try {
	await this.fileService.writeFile(resource, VSBuffer.fromString(content), { etag, mtime });
	} catch (error) {
	if ((<FileOperationError>error).fileOperationResult === FileOperationResult.FILE_MODIFIED_SINCE) {
	throw new Error(UserConfigurationErrorCode.ERROR_FILE_MODIFIED_SINCE);
	}
	}
	}
	}

[bpasero]

Oh wow, you so smart!