opening large or binary files

[akosyakov]

What it does

fix #3859, fix #4731

Important: the goal of this PR is to prevent a user to crash the browser tab, it does not aim to improve performance in the electron case. See also: #8152 (comment)

• fix #7269: complete support vscode.workspace.fs API #7908 has to be landed
• prompt to open a binary file
• prompt to open a large file
• stream content for Monaco editor to avoid blocking the backend and network communications
• limit incremental file update to several megabytes, it will crash the backend for large files, for such files we have to go with streaming

Out of scope:

• electron: bind disk fs provider directly in the renderer process

How to test

• try to open binary file, for instance generate a tar file
• try to open/save large file: Improve memory usage for large files microsoft/vscode#30180
  • generate medium size ts less than 32 mb, it should be possible to open both in browser and electron
  • generate large size ts more than 32 mb, it should prompt in the browser and can file on load, in electron it should load without the prompt

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

• stream content for Monaco editor

Is monaco supporting streaming now or will we just stream and buffer the content before giving it to the editor?

[akosyakov]

Is monaco supporting streaming now or will we just stream and buffer the content before giving it to the editor?

Yes, there are internal Monaco APIs which allow it: https://code.visualstudio.com/blogs/2018/03/23/text-buffer-reimplementation

[akosyakov]

It works, but it is slow for Electron compare to VS Code. We need to bind DiskFileSystemProvider directly in the electron renderer process as VS Code does, but it requires refactoring of how we fork processes and establish the ipc communication since it should be possible to webpack such code.

[lmcbout]

@akosyakov I try to merge as described in the command line instructions and there are many merge conflicts to solved
git checkout -b ak/opening_large_or_binary_files origin/ak/opening_large_or_binary_files
git merge akosyakov/integrate-vscode-api-7269

[marcdumais-work]

@akosyakov I try to merge as described in the command line instructions and there are many merge conflicts to solved
git checkout -b ak/opening_large_or_binary_files origin/ak/opening_large_or_binary_files
git merge akosyakov/integrate-vscode-api-7269

Can't you just checkout branch ak/opening_large_or_binary_files?
i.e., Assuming the remote is named upstream:

git fetch upstream
git checkout --track upstream/ak/opening_large_or_binary_files

[vince-fugnitto]

I verified the functionality of the pull-request and it works as described 👍

Browser:

• opening a binary file triggers a prompt
• opening a file smaller than the preference value does not trigger a prompt
• opening a file larger than the preference value triggers a prompt
• modifying the preference value is respected

Electron:

• opening a binary file triggers a prompt
• opening a file works like master

[lmcbout]

I only used the branch for the large file, unable to merge with "git merge akosyakov/integrate-vscode-api-7269", it triggers many conflict merge.
Testing Browser and Electron on UBUNTU 18.04
Result:
The preference is applied for the file max size
After trying to open a binary file, if I select "Yes" , I can open any small files in preview after
If I select "No", then I cannot open any files in preview anymore, just if I double click to open in a regular editor works.

[lmcbout]

After trying to open a large file, I forgot to put the error I see on the console log
When selecting "No", the following error is logged on the console:

root ERROR Failed to initialize for 'file:///home/lmcbout/script/file2000000': Error: Unable to read file 'file2000000' (Error: Unable to read file 'file2000000' that is too large to open)
    at new FileOperationError (http://localhost:3000/0.bundle.js:3039:28)
    at FileService.push.../../packages/filesystem/lib/browser/file-service.js.FileService.asFileOperationError (http://localhost:3000/0.bundle.js:1417:34)
    at FileService.push.../../packages/filesystem/lib/browser/file-service.js.FileService.rethrowAsFileOperationError (http://localhost:3000/0.bundle.js:1414:20)
    at FileService.<anonymous> (http://localhost:3000/0.bundle.js:1390:30)
    at step (http://localhost:3000/0.bundle.js:522:23)
    at Object.throw (http://localhost:3000/0.bundle.js:503:53)
    at rejected (http://localhost:3000/0.bundle.js:495:65)
Caused by: Error: Unable to read file 'file2000000' that is too large to open
    at new FileOperationError (http://localhost:3000/0.bundle.js:3039:28)
    at FileService.push.../../packages/filesystem/lib/browser/file-service.js.FileService.validateReadFileLimits (http://localhost:3000/0.bundle.js:1477:23)
    at FileService.<anonymous> (http://localhost:3000/0.bundle.js:1461:30)
    at step (http://localhost:3000/0.bundle.js:522:23)
    at Object.next (http://localhost:3000/0.bundle.js:503:53)
    at fulfilled (http://localhost:3000/0.bundle.js:494:58)

Then I can open any small file after that in preview mode as I said earlier

[a1994846931931]

I tried to open a large binary file, and the prompt showed twice (one told me the file is too large, after clicking yes, the other told me that the file is a binary), which seems a little bit annoying to me. Maybe you could come up with a better user experience for situation like this?

[akosyakov]

thanks for testing, it seems to be that FileResource is not a god place for checks, will try to move to another place. I'm wrong the root of issue is the editor preview widget which tries to open an editor differently multiple times and does not handle exceptions properly.

[akosyakov]

@lmcbout @a1994846931931 Could you try again? I've fixed issues with the editor preview extension. There is still some weirdness but it is out of the scope of this PR: #8168

[lmcbout]

After trying to open a binary file, if I select "Yes" | "No" , I can open any small files in preview after 👍 FIX

As @a1994846931931 mentioned, If I try to open a large file and it happens that this file is an executable, there are two popup dialogs, we should only have one popup.

[akosyakov]

As @a1994846931931 mentioned, If I try to open a large file and it happens that this file is an executable, there are two popup dialogs, we should only have one popup.

I agree that UX wise is not really good, but I don't feel that it is right to open a binary file if one agreed to open a large file. Unfortunately I cannot to do checks together to add have only one prompt, since the size check is cheap I need only the stat, but to check for binary content I had to actually open the file and fetch in the worst case more than 100kb. I would prefer to keep it like that for now.

[lmcbout]

I agree that UX wise is not really good, but I don't feel that it is right to open a binary file if one agreed to open a large file. Unfortunately I cannot to do checks together to add have only one prompt, since the size check is cheap I need only the stat, but to check for binary content I had to actually open the file and fetch in the worst case more than 100kb. I would prefer to keep it like that for now.

UX wise not great, I agree. We should not try to open executable file , so I agree to accept this PR as it is right now.
Thanks @akosyakov for your work