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

Better timeout detection in web UI uploads + chunked uploads #28831

Merged
merged 2 commits into from
Sep 4, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Aug 29, 2017

Description

Detects timeouts during web UI uploads and displays a notification when it happens.
Also prevents the global ajax error detection to prevent the full page reload.

Related Issue

Motivation and Context

How Has This Been Tested?

  • TEST: Set client-side "timeout: 5000" in file-upload.js's fileUploadParam. Add sleep(500) in OCA\DAV\Connector\Sabre\File::put(). Upload < 10 mb file. Notification about timeout appears.
  • TEST: Set client-side "timeout: 5000" in file-upload.js's fileUploadParam. Upload 100 mb file. Notification about timeout appears.
  • TEST: IE11: Add sleep(500) in apps/dav/lib/Upload/UploadFolder::createFile(), upload 100 mb file. Notification appears after 2 minutes
  • TEST: IE11: Add sleep(500) in apps/dav/lib/Upload/AssemblyStream::stream_open(). Upload 100 mb file, wait. Notification appears after 2 minutes of stalling at the end of the progress bar.
    => cannot test, it seems IE11 doesn't have a timeout on MOVE, only on PUT

Note: remove extra modifications between each test.

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 development milestone Aug 29, 2017
@PVince81 PVince81 self-assigned this Aug 29, 2017
@PVince81 PVince81 requested a review from VicDeo August 29, 2017 12:18
@PVince81 PVince81 added the p2-high Escalation, on top of current planning, release blocker label Aug 30, 2017
@VicDeo
Copy link
Member

VicDeo commented Aug 31, 2017

it seems to break #28842

@PVince81
Copy link
Contributor Author

@VicDeo details please: steps to reproduce, etc

@PVince81
Copy link
Contributor Author

note that the progress bar has an additional bug I found which I fixed here in additional commits: #28692.

I suggest to first review this one then I'll rebase this timeout madness.

@IljaN IljaN self-requested a review September 1, 2017 12:45
@PVince81
Copy link
Contributor Author

PVince81 commented Sep 1, 2017

Rebased. @VicDeo please recheck

@IljaN
Copy link
Member

IljaN commented Sep 1, 2017

The tests decribed in the description work so far

@VicDeo
Copy link
Member

VicDeo commented Sep 1, 2017

@PVince81 it just stuck in a few seconds state. I can't reproduce it after a rebase so feel free to disregard.

@PVince81
Copy link
Contributor Author

PVince81 commented Sep 4, 2017

@VicDeo @IljaN so does this count as a positive review or is there anything else to address ?

@IljaN
Copy link
Member

IljaN commented Sep 4, 2017

If @VicDeo has no more objections then 👍

@PVince81
Copy link
Contributor Author

PVince81 commented Sep 4, 2017

stable10: #28896

@lock
Copy link

lock bot commented Aug 2, 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 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - To Review p2-high Escalation, on top of current planning, release blocker status/STALE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants