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

non-blocking file upload API (formdata based) #5086

Closed
wants to merge 1 commit into from

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented May 7, 2019

It's breaking to redesign APIs. I've updated CHANGELOG with a record for it.

TODO:

  • firefox
    • bogus percentage
    • often bad request - it's a Gitpod proxy issue, i don't get it locally
    • bogus progress styles
  • linux
    • upload to the same device (cross-device link not permitted, i.e. from /tmp to /home)
  • minimize sync fs access
  • retry on network failures
  • electron
    • remove upload
    • remove download

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from d6c3090 to 53fd418 Compare May 7, 2019 10:57
@akosyakov akosyakov requested review from kittaakos and AlexTugarev May 7, 2019 10:59
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 53fd418 to 6559489 Compare May 7, 2019 11:02
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 6559489 to f696225 Compare May 7, 2019 11:20
@lmcbout
Copy link
Contributor

lmcbout commented May 7, 2019

@akosyakov Question: When I Drag & Drop Theia into a workspace, there are many files , but I get the following error:

root ERROR Error: Upload Failed: Network Failure
at XMLHttpRequest.cb (http://localhost:3000/2.bundle.js:462:51)

I noticed that the UI did not freeze while transferring, The UI responded, I open a file and was able to navigate while the transfer occurred, but the Dropping files stopped at around 40%. Is there a limit for the number of files we can Drag & Drop ?

@akosyakov
Copy link
Member Author

akosyakov commented May 7, 2019

Is there a limit for the number of files we can Drag & Drop ?

@lmcbout No, it seems to be caused by network failure, so upload could not recover afterwards. I will try to reproduce.

Copy link
Contributor

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

Works nicely! 🎉
Thanks!

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from f696225 to 3c6da2b Compare May 8, 2019 06:40
@akosyakov
Copy link
Member Author

akosyakov commented May 8, 2019

@lmcbout i was able to reproduce, unfortunately an http request is hanging after reconnection. I would try to timeout it in order to fail upload.

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 3c6da2b to 2d6be38 Compare May 8, 2019 07:41
@akosyakov
Copy link
Member Author

@lmcbout please try again, i've added a timeout of 5s which gets reset till the browser keeps sending upload progress reporting, so uploading of one big file still should be possible

@lmcbout
Copy link
Contributor

lmcbout commented May 8, 2019

@akosyakov I tried with your latest change and when I tried to upload Theia project to a new workspace and open files from another folder in my workspace, the following error occured:

root ERROR Error: Upload Failed: Network Failure
at rejectRequest_1 (http://localhost:3000/2.bundle.js:468:47)
at XMLHttpRequest.cb (http://localhost:3000/2.bundle.js:488:29)

@akosyakov
Copy link
Member Author

akosyakov commented May 8, 2019

@lmcbout it means that there are issues with an internet connection between you and the server. Where do you try? Do you have a proxy between you and the server with some limitations? Check in the chrome network tab why an upload request failed. There should be an http response from a proxy/server. You should see why it happens. I don't think it can be fixed in the code.

@akosyakov
Copy link
Member Author

@lmcbout i cannot reproduce it. What is your local machine? Could it be that it is not powerful enough to handle upload and other UI actions in parallel in the backend, so that a browser does not receive a response from a local server?

@akosyakov
Copy link
Member Author

@elaihau @vince-fugnitto Could you try as well? Upload theia and at the same time navigate through other files. I want to understand whether it is a general issue or an issue with a particular dev machine.

@lmcbout can you try in Gitpod?

@elaihau
Copy link
Contributor

elaihau commented May 8, 2019

from my dev machine (ubuntu 16 + chrome), i tried (a few times) drag and drop theia folder into my workspace. Colors in Chrome became dark, and then Chrome crashed.

@akosyakov
Copy link
Member Author

akosyakov commented May 8, 2019

@elaihau Which chrome version? and if you do it Gitpod, the same?

@elaihau
Copy link
Contributor

elaihau commented May 8, 2019

Version 73.0.3683.103 (Official Build) (64-bit)

@akosyakov
Copy link
Member Author

My is 74.0.3729.131 (Official Build) (64-bit). It can crash if all files are loaded in memory, like the whole theia project, but code is chunking it with each chunk of 64Mb size. It can be that some version of a browser does not implement lazy file loading properly, so then it won't be possible to upload big amount of files. I am not even sure that there is a way to prevent a browser load everything into the memory then.

@lmcbout
Copy link
Contributor

lmcbout commented May 8, 2019

My Google version is: Version 72.0.3626.121 (Official Build) (64-bit)
It also crashed in my environment as well

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 2d6be38 to 514ebc0 Compare May 8, 2019 20:00
@akosyakov
Copy link
Member Author

@lmcbout i've increased a timeout to 30s and removed all sync access to fs during upload on the backend. Could you try please whether it makes the difference?

@akosyakov
Copy link
Member Author

But in Gitpod all fine? If it is so then it won’t be a browser issue.

I was able to reproduce an issue with failed connection locally with Firefox. Browser gets very busy transferring files and does not give a chance to backend to process them, so connection gets unresponsive.

In practice you won’t upload to your local machine, so backend will stay responsive. It does not make much sense to test it locally.

Please check that uploading files up to 60 mb is still possible as on master. Preferably in Gitpod.

@akosyakov
Copy link
Member Author

Also please use latest Chrome, yours are outdated.

@akosyakov akosyakov changed the title non-blocking file upload API WIP non-blocking file upload API May 9, 2019
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch 2 times, most recently from ecf2d25 to b24ea74 Compare May 9, 2019 09:54
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch 2 times, most recently from 1ef5064 to 61f460a Compare May 9, 2019 11:45
@akosyakov
Copy link
Member Author

@lmcbout I've added retry 5 times on network failures for each chunk.

@akosyakov akosyakov requested a review from geropl May 9, 2019 11:46
@kittaakos
Copy link
Contributor

kittaakos commented May 9, 2019

WIth a local setup, I can upload a file from the workspace to the workspace. 🤦‍♂

I guess it runs into a loop because the Uploading Files... is showing 100% for a while now, and the console is full of:

at FileUploadService.<anonymous> (http://localhost:3000/2.bundle.js:476:51)
root WARN Error: Possible Emitter memory leak detected. 30 exit listeners added. Use event.maxListeners to increase limit
    at Emitter.../../packages/core/lib/common/event.js.Emitter.checkMaxListeners (http://localhost:3000/bundle.js:98865:26)
    at MutableToken._event.Object.assign.maxListeners (http://localhost:3000/bundle.js:98832:27)
    at FileUploadService.<anonymous> (http://localhost:3000/2.bundle.js:546:27)
    at step (http://localhost:3000/2.bundle.js:66:23)
    at Object.next (http://localhost:3000/2.bundle.js:47:53)
    at http://localhost:3000/2.bundle.js:41:71
    at new Promise (<anonymous>)
    at push.../../packages/filesystem/lib/browser/file-upload-service.js.__awaiter (http://localhost:3000/2.bundle.js:37:12)
    at FileUploadService.push.../../packages/filesystem/lib/browser/file-upload-service.js.FileUploadService.doSubmitForm (http://localhost:3000/2.bundle.js:510:16)
    at FileUploadService.<anonymous> (http://localhost:3000/2.bundle.js:476:51)

Another thing, super minor though, Upload Files... does nothing at all in electron. No errors, nothing. If it is not supported, we should consider disabling the command.

@kittaakos
Copy link
Contributor

kittaakos commented May 9, 2019

WIth a local setup, I can upload a file from the workspace to the workspace. 🤦‍♂

It turned out, it happens with a normal use case in both FF and Chrome.

@lmcbout
Copy link
Contributor

lmcbout commented May 9, 2019

@akosyakov My test was to add Theia into a folder by using Drag & Drop

I updated Chrome to : Version 74.0.3729.131 (Official Build) (64-bit)
The upload files was flickering between 0 to 1 %

I also tried on Firefox, I got a different behavior and at the end it stops after a few retry. It uploaded files up to 24% until it timed out. We have many retry now, but it seems we cannot upload a project like Theia into a workspace
image

When trying to load a 20 Meg file, Firefox and Chrome report a loop in the message from 0 to 100% and start over and over again, but never ends. See little video.
FirefoxLoad20M

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 61f460a to 01a7087 Compare May 9, 2019 15:17
@akosyakov
Copy link
Member Author

@lmcbout sorry, i introduced some bugs today, just pushed a fix for them, please try again, also @geropl @kittaakos

on FireFox the state is still pretty bad, requests are just sometimes bogus, i'm trying to understand why. Also performance of FireFox compare to Chrome is very bad at least on mac.

@elaihau
Copy link
Contributor

elaihau commented May 9, 2019

I tested with the latest change on Chrome v74
and observed the following when drag and drop theia folder into my workspace:

  • progress indicator showed the progress went from 0% to 46%, then
  • status bar of theia became yellow, then
  • message showed up a few times saying request timed out, will retry in x seconds, and finally
  • a message showed up, saying the request timed out. at this point, theia status bar remained yellow, and chrome stopped responding (even in other opened tabs)

@akosyakov
Copy link
Member Author

akosyakov commented May 9, 2019

@elaihau locally or in Gitpod? Did you try a file of 60 Mb too?

@elaihau
Copy link
Contributor

elaihau commented May 9, 2019

sorry for the confusion, i tested locally

@akosyakov
Copy link
Member Author

akosyakov commented May 9, 2019

@elaihau as i've explained local test does not make sense, see here: #5086 (comment)

I was able to reproduce an issue with failed connection locally with Firefox. Browser gets very busy transferring files and does not give a chance to backend to process them, so connection gets unresponsive.

In practice you won’t upload to your local machine, so backend will stay responsive. It does not make much sense to test it locally.

I will remove download/upload commands for electron.

@elaihau
Copy link
Contributor

elaihau commented May 9, 2019

ok, i tested on gitpod, and saw almost the same behaviours. only difference is the "time out" messages started to show up at 26%.

@lmcbout
Copy link
Contributor

lmcbout commented May 9, 2019

@akosyakov
I also tested on a UNIX environment with Chrome:

  • Big file : ~80 Meg and the transfer worked fine
  • Project: Theia, I tried with Gitpod , created a new folder, then I tried to Drag & Drop " Theia " project to it. It transferred for a while, then it stops. But the bad thing here, my Chrome did not respond on any tabs after that, I had to kill all process associated to Chrome before being able to use it again

@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from 01a7087 to e057e96 Compare May 10, 2019 08:47
@akosyakov
Copy link
Member Author

akosyakov commented May 10, 2019

@elaihau @lmcbout I've found a memory leak in Chrome via request listeners, now they should be disposed when a request is done. Could you try again whether it makes a difference for you?

Signed-off-by: Anton Kosyakov <[email protected]>
@akosyakov akosyakov force-pushed the ak/non_blocking_file_upload branch from e057e96 to e507a0b Compare May 10, 2019 13:19
@akosyakov akosyakov changed the title WIP non-blocking file upload API non-blocking file upload API May 10, 2019
@elaihau
Copy link
Contributor

elaihau commented May 10, 2019

This is related but probably belongs to another task & PR:

while files are being uploaded / downloaded, do we want to block users from switching from one git branch to another?

@elaihau
Copy link
Contributor

elaihau commented May 10, 2019

Tested in gitpod. I had exactly the same problem that Jacques and I reported yesterday.

@kittaakos
Copy link
Contributor

do we want to block users from switching from one git branch to another?

I do not think so.

@akosyakov
Copy link
Member Author

while files are being uploaded / downloaded, do we want to block users from switching from one git branch to another?

I don't think so either, user can cancel it.

Related to issues:

  • Unfortunately, I cannot reproduce Chrome issues, someone who can reproduce has to profile and see why Chrome does not release the memory. I need a snapshot after couple upload requests finished. @elaihau Could you do it please and upload here? Please don't forget to collect garbage before snapshot.
  • In FireFox crashes are reproducible: although there is no memory leak in the page process, the main FireFox does not release memory with many upload requests. It looks like a bug in FireFox. I suggest we just limit amount of request to one here.

@lmcbout
Copy link
Contributor

lmcbout commented May 10, 2019

I tested the latest commit (e507a0b) with large files (80M and 120M) result: file transfered and able to open it, but the client stop responding for a short period (can see the bottom toolbar showing Offline) but it recovers.
Try multiple files (Theia project ) on Chrome and Firefox

  • Chrome: same problem, freeze Chrome completely and need to restart chrome to access any tab view after
  • Firefox, Stop with less data transfer, but the browser does not freeze, it stays available for using any tabs view after.

@akosyakov
Copy link
Member Author

akosyakov commented May 13, 2019

I am going to give up on FormData based implementation. Unfortunately some version of browsers on some platforms behave bogusly leading to memory leaks in main browser process overtime. Also since XMLHttpRequest does not allow streaming it has memory limitations for different browsers: not more than 2G at max.

I will go with chunking and streaming over the web socket, similar to https://github.com/mozilla/send. It will have very low memory footprint on both sides and allow to upload limitless amount of data. Hope i won't need to compromise UI responsiveness and upload speed for it too much.

@akosyakov akosyakov changed the title non-blocking file upload API non-blocking file upload API (formdata based) May 13, 2019
@akosyakov akosyakov deleted the ak/non_blocking_file_upload branch June 13, 2019 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants