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

Use chunking in web upload #26306

Closed
wants to merge 6 commits into from
Closed

Use chunking in web upload #26306

wants to merge 6 commits into from

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Oct 7, 2016

Description

Switch on chunking for files bigger than 10 MB.
Some additional fixes to make it use the correct URL/client.

Related Issue

Fixes #8955

Motivation and Context

Makes it possible to avoid timeouts when uploading bigger files.
Might also make it resumable (still to be tested)

How Has This Been Tested?

For all tests, check the network console:

  • TEST: upload small file, no chunking endpoint used
  • TEST: upload file bigger than 10 MB, see chunking endpoint used, multiple call mades followed by final MOVE. Then file appears in the UI.
  • TEST: chunking not enabled for public upload even for big files
  • TEST: with IE11 which is one of the main reasons for this fix
  • TEST: upload + overwrite
  • TEST: mtime preserved on chunked upload
  • TEST: uploads transfer folder is deleted in case of error or abort (you can see a DELETE call happening at the end)

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PVince81 PVince81 added this to the 9.2 milestone Oct 7, 2016
@mention-bot
Copy link

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @luckydonald and @DeepDiver1975 to be potential reviewers.

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 7, 2016

Important note

jquery blueimp doesn't support parallel uploading of chunks, so the chunks are always uploaded sequentially. This isn't too bad as it still brings the benefit of avoiding timeouts.
In the future we should look into using a different library (ex: resumable.js) and/or implement the chunking algo by ourselves.

@DeepDiver1975
Copy link
Member

tested - works 👍

@PVince81
Copy link
Contributor Author

  • BUG: simulating a failed upload triggers the global ajax error handlers which causes a full page reload

@PVince81
Copy link
Contributor Author

PVince81 commented Nov 4, 2016

Hmm this is tricky because if you test it by making the server unavailable, there are other ajax calls (notifications, getstoragestats) that will fail too.

I tried disabling the reload and it seems that jquery.fileupload doesn't automatically resume.

@michaelstingl
Copy link

@PVince81 I need confirmation that this will work in IE11. How could I verify this asap?

@butonic
Copy link
Member

butonic commented Jan 18, 2017

When the connection is closed the Response status is 0, which triggers the reload. Reloading on Status = 0 was introduced with #16783 (comment) because of a missing Access-Control-Allow-Origin header on the IdP. The proper fix is to make the IdP send this header.

While we currently cannot detect an abort we might ba able to configure mod_shib to send 403 forbidden for xhr requests. That is what mod_mellon does: https://github.com/UNINETT/mod_auth_mellon/blob/master/README#L169-L173

Either test mod_mellon or try to make apache send 403 responses when no shibboleth session is present for xhr requests (X-Request-With: XMLHttpRequest). The latter might be very tricky ...

see https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS
see http://charliedigital.com/2015/07/17/adventures-in-single-sign-on-cross-domain-script-request/
see https://github.com/monmohan/cors-experiment/blob/master/docs/CORS.md

@butonic
Copy link
Member

butonic commented Jan 18, 2017

That being said ... I can live with the page refreshing too often... I cannot live with IE11 failing to upload files >2.2GB.

@michaelstingl
Copy link

Upload with 32-bit IE11 worked well (In my example 2.281.105.750 bytes). Progress bar shows very wrong estimated values in the range from days to seconds. It took some time after the progress bar disappeared and the file appeared.

@butonic
Copy link
Member

butonic commented Jan 18, 2017

Tested in IE11:
ie11 chunks

Backport to 9.1 upcoming

@butonic
Copy link
Member

butonic commented Jan 18, 2017

✔️ with 4.6GB on IE11 🎉 @PVince81 is awesome!

@PVince81
Copy link
Contributor Author

There's a potential bug, see the discussion here owncloud/client#5476 (comment).

Need to make sure that the uploaded chunk names are ascii-sortable in the correct order to avoid corruption because the endpoint expects this.

@PVince81
Copy link
Contributor Author

PVince81 commented Jan 19, 2017

  • make sure the chunks have the proper order

@PVince81
Copy link
Contributor Author

PVince81 commented Jan 19, 2017

@PVince81
Copy link
Contributor Author

PVince81 commented Mar 20, 2017

  • DELETE transaction if the user cancels

@PVince81
Copy link
Contributor Author

  • BUG: no progress shown during final move

At this stage no progress can be shown because the transfer already ended. So one idea would be to already insert the entry into the file list but let it appear with a spinner.

@PVince81
Copy link
Contributor Author

If we had a good solution to get rid of the IDP "status=0" fail detection it would be great, because that check is messing up with more stuff. #26306 (comment)

@PVince81 PVince81 modified the milestones: 10.0.1, 10.0 Mar 30, 2017
@PVince81
Copy link
Contributor Author

PVince81 commented Mar 31, 2017

  • BUG: cannot work on public link page because the "uploads" endpoint only accepts logged in sessions.

Will need further work there.
This also means that anonymous upload with chunking will not work either.

@PVince81 PVince81 modified the milestones: 10.0.2, 10.0.3 May 26, 2017
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 4, 2017

cannot work on public link page because the "uploads" endpoint only accepts logged in sessions.

this is a major blocker. Need to talk with @DeepDiver1975 how to redesign the DAV endpoints to allow public access

@PVince81 PVince81 added the p1-urgent Critical issue, need to consider hotfix with just that issue label Jul 4, 2017
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 7, 2017

Let's finish this without the public link part then.

@PVince81 PVince81 self-assigned this Jul 11, 2017
@felixboehm
Copy link
Contributor

felixboehm commented Jul 12, 2017

Any blockers?

Open separat issue for discussion on public link?

@PVince81
Copy link
Contributor Author

No blocker, just need to find time to finish this...

The biggest missing bit now is error handling in case of abort.

@mmattel
Copy link
Contributor

mmattel commented Jul 13, 2017

@PVince81 Thanks in advance and 👍

@PVince81
Copy link
Contributor Author

Giving up on trying to make it retry chunks. This jquery.fileupload lib is really painful to use.
For example it triggers the upload "fail" event before the "chunkfail" event and resuming doesn't seem to work with this workflow. I'm sure that if we invest enough time we might be able to find the right code gymnastics to make it work properly...
Another challenge is detecting whether the 500 returned by the server is actually a timeout error and we should try resuming or whether it's a legit error and we need to abort.

But for now let's focus on getting this merged. The main purpose for now is to make it possible to upload files bigger than 2 GB in IE11.

@PVince81
Copy link
Contributor Author

  • BUG: progress bar disappears too early while the final MOVE is in progress

This would also require hacking around jquery.fileupload since that one thinks we're already done since the last chunk was uploaded already... The lib isn't really compatible, too much hacking... Maybe one day we should find another lib or develop our own.

@PVince81
Copy link
Contributor Author

Jenkins is kaputt:

12:57:30 ERROR: Error fetching remote repo 'origin'
12:57:30 hudson.plugins.git.GitException: Failed to fetch from https://github.com/owncloud/core.git

Resent PR separately: #28415

@PVince81 PVince81 closed this Jul 17, 2017
@PVince81 PVince81 deleted the webui-uploadchunking branch July 17, 2017 11:51
@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
2 - Developing app:files p1-urgent Critical issue, need to consider hotfix with just that issue sev1-critical
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Based File Upload Chunking
10 participants