Backup file system provider can not do buffered write

[rebornix]

Does this issue occur when all extensions are disabled?: Yes/No

• VS Code Version: 1.83-Insiders
• OS Version: macOS

I was verifying #192970 and found that large file piece tree buffers are still merged before write. Turns out that backup is always doing doWriteUnbuffered since it's using FileUserDataProvider, which doesn't have FileOpenReadWriteClose capability so it always runs

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

Lines 394 to 395 in 3b60c12

	await this.doWriteUnbuffered(provider, resource, options, bufferOrReadableOrStreamOrBufferedStream);
	}

and unfortunately unbuffered write always concatenates all buffers first

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

Line 1294 in 3b60c12

buffer = readableToBuffer(bufferOrReadableOrStreamOrBufferedStream);

[rebornix]

One interesting behavior I found is sometimes backup happens right after I press cmd+s to save, maybe the delayed backup kicks in right before I save, but it feels weird to have both a backup and save operation at the same time.

[bpasero]

Good catch, but I think this is simply something that was forgotten when introducing the file user data provider. Since the underlying disk file system provider supports this, I see no reason why the file user data provider could not support this.

@sandy081 can you check?

[bpasero]

Looks like this was an explicit decision in 87bf1d8#diff-28073e52c2a7e44ac456dfb32c4f0794985cb4a4bcc7c0d99a033e8224fff52cR25 and probably done to enforce atomic writes.

I think we may be fine to bring back that capability. The file service is already optimising the calls to the provider in a way that writeFile is being called when the buffer is not too large (3*64kb I think):

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

Lines 372 to 390 in 2553649

	// optimization: if the provider has unbuffered write capability and the data
	// to write is not a buffer, we consume up to 3 chunks and try to write the data
	// unbuffered to reduce the overhead. If the stream or readable has more data
	// to provide we continue to write buffered.
	let bufferOrReadableOrStreamOrBufferedStream: VSBuffer | VSBufferReadable | VSBufferReadableStream | VSBufferReadableBufferedStream;
	if (hasReadWriteCapability(provider) && !(bufferOrReadableOrStream instanceof VSBuffer)) {
	if (isReadableStream(bufferOrReadableOrStream)) {
	const bufferedStream = await peekStream(bufferOrReadableOrStream, 3);
	if (bufferedStream.ended) {
	bufferOrReadableOrStreamOrBufferedStream = VSBuffer.concat(bufferedStream.buffer);
	} else {
	bufferOrReadableOrStreamOrBufferedStream = bufferedStream;
	}
	} else {
	bufferOrReadableOrStreamOrBufferedStream = peekReadable(bufferOrReadableOrStream, data => VSBuffer.concat(data), 3);
	}
	} else {
	bufferOrReadableOrStreamOrBufferedStream = bufferOrReadableOrStream;
	}

On top of that: if any component comes with a buffer for writing, we never stream:

vscode/src/vs/workbench/services/textfile/browser/textFileService.ts

Lines 267 to 275 in 2553649

	async write(resource: URI, value: string | ITextSnapshot, options?: IWriteTextFileOptions): Promise<IFileStatWithMetadata> {
	const readable = await this.getEncodedReadable(resource, value, options);
	if (options?.writeElevated && this.elevatedFileService.isSupported(resource)) {
	return this.elevatedFileService.writeFileElevated(resource, readable, options);
	}
	return this.fileService.writeFile(resource, readable, options);
	}

Which I would assume is the case for all our settings when written to programmatically.

[sandy081]

@bpasero Does it also need FileUserDataProvider to support FileSystemProviderCapabilities.FileOpenReadWriteClose ?

[bpasero]

@sandy081 yes, that is what the IFileService will probe for before deciding which method to use. Since yours does not declare it, all writes are without streaming.

[rebornix]

I experimented adding the FileOpenReadWriteClose capability to the FileUserDataProvider #193228, with this backup streams again and no longer blocks the UI. Feel free to discard my PR if it's in the wrong direction

[bpasero]

I wonder if a better approach for this would be to drop the FileUserDataProvider and simply expose a method on IFileService to set options for certain paths.

Basically this method would call into IFileService instead of requiring to subclass the disk file system provider:

vscode/src/vs/platform/userData/common/fileUserDataProvider.ts

Lines 54 to 62 in fa99dac

	private updateAtomicWritesResources(): void {
	this.atomicWritesResources.clear();
	for (const profile of this.userDataProfilesService.profiles) {
	this.atomicWritesResources.add(profile.settingsResource);
	this.atomicWritesResources.add(profile.keybindingsResource);
	this.atomicWritesResources.add(profile.tasksResource);
	this.atomicWritesResources.add(profile.extensionsResource);
	}
	}

[sandy081]

I liked that. Does it mean we should not use vscode-userdata scheme in Desktop?

[bpasero]

@sandy081 I would not suggest to change anything with regards to URI or scheme, just simply add a way on the IFileService to enforce custom options when writing for a set of Uri. Its a bit of a hack, but I find the custom file system provider to cause more issues. I would assume you can drop it then.

[sandy081]

I am wondering it is not straight forward to use disk FSP without changing the scheme. The main purpose of vscode-userdata fsp is to handle the scheme, for eg., watching on user data scheme and getting change events on the same scheme. We should also adopt the disk FSP not to call uri.fsPath directly which I saw using it here

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

Lines 217 to 219 in d8e6919

	protected toFilePath(resource: URI): string {
	return normalize(resource.fsPath);
	}

[bpasero]

@sandy081 how about this then:

• keep using vscode-userdata scheme as we do today ✅
• keep using a fileUserDataProvider as we do today ✅
• take in the change from Peng in Introduce concept of enforced atomic operations (fix #193151) #193228 to enable streaming support ✅
• revert the change from 3b60c12 ✅
• expose a method on IFileService to enforce atomic writes with the knowledge we have from 3b60c12

I think then we get the best of all worlds:

• we don't change schemes
• we don't change file system providers around fsPath handling
• the decision whether to use atomic write or not moves into the IFileService based on a whithlist and the new API

[sandy081]

Sounds good 👍

[bpasero]

@sandy081 I went with a slightly different approach: since the file user data provider is used in many contexts, it makes more sense to keep the decision about which resources should get atomic read/write inside the provider. As such, I added new methods to enforce atomic read, write or delete: #193228

[vscodenpa]

This bug has been fixed in the latest release of VS Code Insiders!

@rebornix, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version c72447e of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!