Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Large file working copy handling on web #176993

Closed
joyceerhl opened this issue Mar 13, 2023 · 8 comments · Fixed by #177695
Closed

Large file working copy handling on web #176993

joyceerhl opened this issue Mar 13, 2023 · 8 comments · Fixed by #177695
Assignees
Labels
file-io File I/O insiders-released Patch has been released in VS Code Insiders remote Remote system operations issues web Issues related to running VSCode in the web
Milestone

Comments

@joyceerhl
Copy link
Contributor

joyceerhl commented Mar 13, 2023

In the latest pre-release of GitHub Repositories, we support committing files via LFS: microsoft/vscode-remote-repositories-github#7. This means GitHub Repositories (and soon vscode.dev) will become a zero-configuration Git LFS-capable client, which will make it easier to edit and commit to e.g. microsoft/vscode-docs without error (forgetting to install and set up LFS locally).

Currently, the likely workflows that users will use are:

  1. the Upload... action from the explorer context menu to upload a file, then commit via SCM view
  2. paste from clipboard into a markdown file, rename file if necessary, then commit via SCM view

In both these workflows, the user must first create a working copy in the workspace (managed by GitHub Repositories as an entry in IndexedDB via the extension context's global storage location and VS Code FS APIs) before committing. The problem with this is that we would store the full working copy of the file in the browser, and browsers are subject to unknown and potentially severe storage limitations. It seems quite likely that a user might unintentionally exhaust their browser storage limit in this way. Specifically:

  • FireFox browsers are limited to 50% of disk storage
    • a domain group (both vscode.dev and insiders.vscode.dev are one group) may use 20% of this global limit
  • There are separate limits for Chrome and it is possible to have a quota of 0

Note that this problem also occurs if a user happens to be editing a large file in vscode.dev with GitHub Repositories that is not LFS-tracked, so the problem is not LFS-specific and already exists today.


One suggestion was to implement some form of streaming upload gesture in the SCM view which would immediately commit and push an LFS file to the remote repository, bypassing having to write an LFS file to browser storage and only then commit it to the remote. However, this is incompatible with workflow number 2 above, which is a smooth workflow for adding both an image and a markdown link reference in one gesture, and is less natural as well. There are also UX issues, e.g. we would prompt the user for a commit message before making the commit.

I'm filing this issue to discuss how we can ensure that users who edit large files in vscode.dev with GitHub Repositories do not accidentally run out of browser storage. In particular:

  • Do we need a special setting on web for file size limits that is more conservative than the limit in desktop?
  • Do we handle the QuotaExceededError which is thrown when an origin exhausts the group origin policy even after group origin eviction has occurred? At minimum we should provide an error message, we can also suggest Reopen in Desktop with GitHub Repos which will at least circumvent the browser-imposed limit

    Note: If the group limit is exceeded, or if origin eviction couldn't free enough space, the browser will throw a QuotaExceededError.

@joyceerhl
Copy link
Contributor Author

Ben, I assigned you to start because I see you are involved in related discussions #172129 and #167719, please let me know if I should reach out to someone else!

@bpasero
Copy link
Member

bpasero commented Mar 15, 2023

@joyceerhl

Do we need a special setting on web for file size limits that is more conservative than the limit in desktop?

We have a configurable limit that we recently introduced to not open large files by accident and the limit is 10mb in remote and 50mb in web environments:

export function getLargeFileConfirmationLimit(arg?: string | URI): number {
const isRemote = typeof arg === 'string' || arg?.scheme === Schemas.vscodeRemote;
const isLocal = typeof arg !== 'string' && arg?.scheme === Schemas.file;
if (isLocal) {
// Local almost has no limit in file size
return 1024 * ByteSize.MB;
}
if (isRemote) {
// With a remote, pick a low limit to avoid
// potentially costly file transfers
return 10 * ByteSize.MB;
}
if (isWeb) {
// Web: we cannot know for sure if a cost
// is associated with the file transfer
// so we pick a reasonably small limit
return 50 * ByteSize.MB;
}
// Local desktop: almost no limit in file size
return 1024 * ByteSize.MB;
}

Would that suffice?

Do we handle the QuotaExceededError which is thrown when an origin exhausts the group origin policy even after group origin eviction has occurred? At minimum we should provide an error message, we can also suggest Reopen in Desktop with GitHub Repos which will at least circumvent the browser-imposed limit

That depends on the file system provider that uses IndexedDB as backend. I am not sure who implemented that, maybe @sandy081? The workbench cannot handle these errors in the file service because each provider may implement this differently.

@bpasero bpasero added under-discussion Issue is under discussion for relevance, priority, approach remote Remote system operations issues file-io File I/O web Issues related to running VSCode in the web labels Mar 15, 2023
@sandy081
Copy link
Member

sandy081 commented Mar 16, 2023

Yes this has to be handled by the FSP.

@bpasero May I know what is expected when there is not enough space on the disk for the FSP to write. For eg., what does node does in this case?

@bpasero
Copy link
Member

bpasero commented Mar 16, 2023

Node.js will throw an error, probably directly passing it through from the lower level OS call. It is up to the caller of the method how to deal with the error.

@joyceerhl
Copy link
Contributor Author

@bpasero that setting is only observed for opening files today, would it make sense to observe it in the Upload action as well?

@bpasero
Copy link
Member

bpasero commented Mar 17, 2023

I am not sure, the setting is really only for editors opening, not so much general file IO transfer limitations. The original intent was to limit the possible network cost opening a large editor could have by accident because we immediately open a file when you click it. The setting is also called workbench.editorLargeFileConfirmation.

I think the file system provider should stat the file and refuse to do the operation if the available space is known to be exceeded?

For example if there is a IndexedDB limit of 5 MB, then large files should maybe not be accepted at all on the layer of the provider.

@joyceerhl
Copy link
Contributor Author

joyceerhl commented Mar 17, 2023

That makes sense. I think we can use navigator.storage.estimate--sadly Safari does not support. I am not sure how the performance of this API holds up though.

async writeFile(resource: URI, content: Uint8Array, opts: IFileWriteOptions): Promise<void> {
try {
const existing = await this.stat(resource).catch(() => undefined);
if (existing?.type === FileType.Directory) {
throw ERR_FILE_IS_DIR;
}
await this.bulkWrite([[resource, content]]);
} catch (error) {
this.reportError('writeFile', error);
throw error;
}
}

would instead be

const ERR_FILE_EXCEEDS_MEMORY_LIMIT = createFileSystemProviderError(localize('fileExceedsMemoryLimit', "File exceeds memory limit"), FileSystemProviderErrorCode.FileExceedsMemoryLimit);


async writeFile(resource: URI, content: Uint8Array, opts: IFileWriteOptions): Promise<void> {
	try {
		const existing = await this.stat(resource).catch(() => undefined);
		const estimated = await navigator?.storage?.estimate?.();
		const memoryLimit = (estimated?.quota ?? 0) - (estimated?.usage ?? 0);
		if (existing?.type === FileType.Directory) {
			throw ERR_FILE_IS_DIR;
		} if ((existing?.size ?? 0) > memoryLimit) {
			throw ERR_FILE_EXCEEDS_MEMORY_LIMIT;
		}
		await this.bulkWrite([[resource, content]]);
	} catch (error) {
		this.reportError('writeFile', error);
		throw error;
	}
}

@bpasero
Copy link
Member

bpasero commented Mar 18, 2023

Sure, feel free to open a PR, maybe consult with @sandy081 who I think owns this provider.

@bpasero bpasero assigned joyceerhl and unassigned bpasero Apr 5, 2023
@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Apr 5, 2023
@bpasero bpasero removed the under-discussion Issue is under discussion for relevance, priority, approach label Apr 5, 2023
@vscodenpa vscodenpa added this to the April 2023 milestone Apr 5, 2023
@vscodenpa vscodenpa added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
file-io File I/O insiders-released Patch has been released in VS Code Insiders remote Remote system operations issues web Issues related to running VSCode in the web
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants