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

[filesystem] improve download and allow downloading of big files and parallel downloads. #4890

Closed
wants to merge 1 commit into from

Conversation

uniibu
Copy link
Contributor

@uniibu uniibu commented Apr 12, 2019

Summary:
This PR improves downloading of files from Theia including:

  • downloading of big files without any limitation(other than the user's disk storage).
  • unique download link.
  • downloads can be paused and/or resumed.
  • allows parallel downloads in cases a user wanted to use a 3rd party downloader.
  • adds Copy Download Link menu to support the above.
  • works on chrome and firefox.
  • works on non-ascii file names and folder names.

TLDR;
Currently, there are two main concerns regarding downloads. One, the client download is handled in the client background via fetch which stores the blob in the memory. There are limitations using this approach, one, blob can only handle a specific size depending on the browser and this is prone to OOM(Out of Memory) error. Two, the backend that serves the file uses fs.readFile which is not very sufficient specially for big files.

To handle the first issue, instead of fetching the resource/file in the backend, the frontend instead makes an initial request to download a file, and the backend 'prepares' it and responds with a unique id to be used to download. The frontend then attaches this unique download link to the anchor tag with a download property and simulates a click. The download is now handled natively by the browser.

For the second issue,the simplest solution was to use a readable stream piped to the response so that the backend does not need to store the whole file in memory before serving it to the user.

Improvements, since this pr now allows downloading big project files/folders, some of us prefer to use a separate downloader which downloads the files in parallel. So I added support for parallel downloads using Range header. And lastly, to be able to use a separate downloader, you will need the download link, so I added a Copy Download Link menu option which works for singe and multiple(tar) files/folders. Cheers!

Signed-off-by: Uni Sayo unibtc@gmail.com

@akosyakov akosyakov requested a review from kittaakos April 12, 2019 08:59
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Wonderful feature, looks really robust!

I tried a few things despite the link not being copied on Linux, and it works great! Single or multiple files, it archives it with what looks like the closest common parent, works in a multi-root workspace as well!

Although in a multi-root workspace, I tried downloading files from two relatively different places, and I ended up with an archive containing 2 archives. It works, but I wonder if this is intended.

Anyway, last but not least, maybe I missed it, but is there a way to list the downloads?
Either a dedicated view with the different links, or maybe just a quick-pick menu that would open the links once we select an item (since we are having troubles with the copy command)

IIUC you do track downloads already: https://github.com/theia-ide/theia/blob/91943cf34eb8837fb631d7c6119d499da07c6d94/packages/filesystem/src/node/download/file-download-collection.ts#L28-L89

I like these changes.

@kittaakos
Copy link
Contributor

The status bar shows, I am offline after copying the download link for a > 4GB file:
Screen Shot 2019-04-16 at 10 31 25

Also, when I copy the download link for multiple files and paste it into the browser, I get
Screen Shot 2019-04-16 at 10 36 37

If copying the download link for multiple files is not supported, we should disable it on the UI.

@uniibu
Copy link
Contributor Author

uniibu commented Apr 16, 2019

If copying the download link for multiple files is not supported, we should disable it on the UI.

Regarding the offline, I've experienced this as well on gitpod, but i have not experienced it on my own server. This is probably something of a server limitation? I'm not 100% sure.

And about the unable to download file, the links actually expire after 1 minute. I don't see any use case of pooling them together for later use, unless you intend to share it to someone else later on which is out of scope of this pr. Another reason for expiring the link is so that, it triggers the clean up of the files generated by archiving multiple files/folders.

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

but i have not experienced it on my own server.

It happened with my local setup.

And about the unable to download file, the links actually expire after 1 minute.

It wasn't related to timeout or something. When I select two nodes from the navigator, I select Copy Download Link and I paste the link to the browser, I got the error.

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I have tried it in Chrome, it works nicely. Thank you for your contributions, @uniibu!
Please, see my comments.

This PR improves the download by letting the native browser handle it
instead of fetching it in the background. This also adds a unique link
which expires in 1 minute. Tested on chrome 73.0 and
firefox 60.5

Signed-off-by: Uni Sayo <unibtc@gmail.com>
@@ -40,8 +37,6 @@ export class FileDownloadService {
@inject(FileSystem)
protected readonly fileSystem: FileSystem;

@inject(StatusBar)
Copy link
Contributor

Choose a reason for hiding this comment

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

From the CI:

[lint] ERROR: /home/travis/build/theia-ide/theia/packages/filesystem/src/browser/download/file-download-service.ts:40:1 - Consecutive blank lines are forbidden

protected anchor: HTMLAnchorElement | undefined;
protected downloadQueue: number[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

All symbols with protected and public (default) visibility are part of the API, if you change it, you have to declare your modifications as a breaking change in the changelog.

@@ -147,54 +142,68 @@ export class FileDownloadService {
return this.deferredUpload.promise;
}

protected handleCopy(event: ClipboardEvent, downloadUrl: string) {
Copy link
Contributor

@kittaakos kittaakos Apr 17, 2019

Choose a reason for hiding this comment

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

All methods and functions with protected and public (default) visibility, must explicitly declare the return type. For instance, here, it is : void.

Please, apply this rule to all the methods you have introduced.

@akosyakov
Copy link
Member

@kittaakos @marechal-p Is it good? Can it be merged?

@kittaakos
Copy link
Contributor

@kittaakos @marechal-p Is it good? Can it be merged?

I can see unresolved conversations, and the build is broken; probably it is not ready for the merge.

@uniibu
Copy link
Contributor Author

uniibu commented May 8, 2019

Hi guys, sorry for the delay, been busy lately with work. Ill try to finish this week. Cheers

@akosyakov
Copy link
Member

Does it address #5197 as well?

@uniibu Do you have plans to work on it? Would you mind if someone else tries to finish it up?

@uniibu
Copy link
Contributor Author

uniibu commented May 28, 2019

@akosyakov Yes for sure. I've been caught up with work and couldn't literally find time right now. So if anyone wants to continue it, it's very much fine with me :) Sorry again guys.

@kittaakos
Copy link
Contributor

So if anyone wants to continue it, it's very much fine with me :)

👍

We continue the work here: #5466

@akosyakov
Copy link
Member

closing it in favor of #5466

@akosyakov akosyakov closed this Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants