Skip to content

Conversation

@rebornix
Copy link
Member

The only two file system providers being passed to FileUserDataProvider is either the disk one or the in memory one. The former already supports IFileSystemProviderWithOpenReadWriteCloseCapability thus in this PR we added the capability to the in memory file system provider

@rebornix rebornix self-assigned this Sep 15, 2023
@rebornix rebornix requested review from bpasero and sandy081 and removed request for sandy081 September 15, 2023 18:22
@vscodenpa vscodenpa added this to the September 2023 milestone Sep 15, 2023
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

LGTM except one code style issue.

I leave it up to @sandy081 to comment on the impact of this change. I think the idea of this provider was to explicitly enforce atomic writes on the level of the file system provider for all user data files (except backups). With your change, we now rely on the file service to use writeFile for this to happen. But writeFile is used only in specific conditions, for example when the file is small (3 * 64kb I believe) or when a file is changed not through an editor that has a ITextSnapshot but programmatically where the entire buffer is passed in (which I believe is most often the case with user data).

Co-authored-by: Benjamin Pasero <benjamin.pasero@microsoft.com>
@rebornix rebornix requested a review from bpasero September 17, 2023 17:27
@sandy081
Copy link
Member

or when a file is changed not through an editor that has a ITextSnapshot but programmatically where the entire buffer is passed in (which I believe is most often the case with user data).

@bpasero Is it also true if programmatic save goes through ITextFileService.save ?

save(resource: URI, options?: ITextFileSaveOptions): Promise<URI | undefined>;

@bpasero
Copy link
Member

bpasero commented Sep 18, 2023

@sandy081 no. But I think a better solution is to stop overriding the file system provider and do what I had suggested in #193151 (comment)

@sandy081
Copy link
Member

In this case, I would not suggest this change. It is important to have atomic write for user data like settings and extensions.

@bpasero bpasero requested review from bpasero and removed request for bpasero September 20, 2023 05:15
@bpasero bpasero enabled auto-merge (squash) September 20, 2023 06:17
@bpasero
Copy link
Member

bpasero commented Sep 20, 2023

This is ready for review.

@bpasero bpasero changed the title Fix #193151. Add IFileSystemProviderWithOpenReadWriteCloseCapability to FileUserDataProvider Introduce concept of enforced atomic operations (fi x#193151) Sep 20, 2023
@bpasero bpasero changed the title Introduce concept of enforced atomic operations (fi x#193151) Introduce concept of enforced atomic operations (fix #193151) Sep 20, 2023
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Good change. I liked it.

@bpasero bpasero merged commit 616dc3a into main Sep 20, 2023
@bpasero bpasero deleted the rebornix/uncertain-tarsier branch September 20, 2023 08:31
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Good change. I liked it.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants