Scale read-timeout on chunk-upload file assembly #709

minfaer · 2021-09-04T17:02:41Z

Fix nextcloud/android#5609
Fix nextcloud/android#8134

This is a fix for nextcloud/android#5609 as proposed in #696.
It works by setting the read timeout for the move-method that assembles chunk-uploads on the server to 3 minutes per GB between 30 seconds and 30 minutes, consistent with Nextcloud-dektop.

Set read timeout for the move-method that assembles chunk-uploads on the server to 3 minutes per GB between 30 seconds and 30 minutes, consistent with Nextcloud-dektop. Signed-off-by: M. Thees <m.thees@intempestatem.eu>

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java

tobiasKaminsky · 2021-09-07T05:24:45Z

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java

                moveMethod.addRequestHeader(E2E_TOKEN, token);
            }
-            int moveResult = client.executeMethod(moveMethod);
+            int moveResult = client.executeMethod(moveMethod, CalcAssembleTimeout(file), -1);


For -1 can you introduce a const, e.g. DO_NOT_CHANGE_DEFAULT, so that when reading the code one directly knows that this value does not change connection timeout.

tobiasKaminsky · 2021-09-07T05:25:08Z

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java

+        final double threeMinutes = 3.0 * 60 * 1000;
+        final int thirtySeconds = 30 * 1000;
+        final int thirtyMinutes = 30 * 60 * 1000;
+        int AssembleReadTimeout = Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );


Suggested change

int AssembleReadTimeout = Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );

return Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );

I'd also suggest extracting file.length() / 1e9 into a new variable named fileSizeInGb to make it easier to read and make the algorithm clearer.

tobiasKaminsky · 2021-09-07T05:27:14Z

@minfaer thanks for your PR 🎉
If you have time it would be really great if you can write some unit tests in

android-library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.java

Line 73 in fba3a49

public void testUploadWithoutExistingChunksSmallChunks() {

Different file sizes:

0b
100b
100 Mb
1 Gb
2 Gb
50 Gb
500 Gb

If you do not have time, then we will take care of it :)

AlvaroBrey · 2021-09-07T08:47:34Z

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java

    }
+
+    private int CalcAssembleTimeout(File file) {
+        final double threeMinutes = 3.0 * 60 * 1000;


I don't think this needs to be a double, an int should work fine.

In the original version, this was a double to prevent threeMinutes * file.length() / 1e9 from truncating to full Gb, but by declaring fileSizeInGb as double, this has indeed become unnecessary.

AlvaroBrey · 2021-09-07T08:48:31Z

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java

+        final double threeMinutes = 3.0 * 60 * 1000;
+        final int thirtySeconds = 30 * 1000;
+        final int thirtyMinutes = 30 * 60 * 1000;
+        int AssembleReadTimeout = Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );


I'd also suggest extracting file.length() / 1e9 into a new variable named fileSizeInGb to make it easier to read and make the algorithm clearer.

AlvaroBrey · 2021-09-07T08:57:35Z

Sorry for the close and reopen, my fingers slipped 🤦 I've added a couple of minor comments. Thanks for your contribution!

Co-authored-by: Tobias Kaminsky <tobias@nextcloud.com> Signed-off-by: M. Thees <m.thees@intempestatem.eu>

Signed-off-by: M. Thees <m.thees@intempestatem.eu>

minfaer · 2021-09-07T11:02:52Z

Thank You @tobiasKaminsky and @AlvaroBrey for Your feedback! I implemented your suggestionsand am looking into writing the unit-tests.

codecov · 2021-09-07T12:08:33Z

Codecov Report

Merging #709 (415acf0) into master (b89ecd3) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 415acf0 differs from pull request most recent head e1c7695. Consider uploading reports for the commit e1c7695 to get more accurate results

@@            Coverage Diff             @@
##           master     #709      +/-   ##
==========================================
- Coverage   44.54%   44.49%   -0.05%     
==========================================
  Files         154      154              
  Lines        6174     6180       +6     
  Branches      813      813              
==========================================
  Hits         2750     2750              
- Misses       3003     3009       +6     
  Partials      421      421

Impacted Files	Coverage Δ
...ources/files/ChunkedFileUploadRemoteOperation.java	`0.00% <0.00%> (ø)`

m-terlinde · 2021-09-30T11:41:05Z

Thanks @minfaer for this suggested patch!
Since this issue is bugging me for month, I'd love to see the patch in production.

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky · 2021-10-18T07:08:50Z

I added tests and once CI is passed, we can merge it.
Thanks @minfaer

nextcloud-android-bot · 2021-10-18T09:31:27Z

Lint

Type	master	PR
Warnings	2	2
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	14
Correctness Warnings	38
Internationalization Warnings	6
Malicious code vulnerability Warnings	7
Multithreaded correctness Warnings	3
Performance Warnings	13
Security Warnings	1
Dodgy code Warnings	40
Total	122

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	14
Correctness Warnings	38
Internationalization Warnings	6
Malicious code vulnerability Warnings	7
Multithreaded correctness Warnings	3
Performance Warnings	13
Security Warnings	1
Dodgy code Warnings	40
Total	122

nextcloud-android-bot · 2021-10-18T09:38:05Z

stable-IT test failed: https://www.kaminsky.me/nc-dev/library-integrationTests/560-IT-stable-09-37

AlvaroBrey · 2021-10-18T11:01:19Z

stable-IT test failed: https://www.kaminsky.me/nc-dev/library-integrationTests/560-IT-stable-09-37

Checked this out locally, nothing to worry about. Thanks for your contribution @minfaer !

szaimen · 2021-10-18T13:30:50Z

🎉

Scale read-timeout on chunk-upload file assembly

1f69428

Set read timeout for the move-method that assembles chunk-uploads on the server to 3 minutes per GB between 30 seconds and 30 minutes, consistent with Nextcloud-dektop. Signed-off-by: M. Thees <m.thees@intempestatem.eu>

minfaer force-pushed the FixAssembleTimeout branch from 4a6c62d to 1f69428 Compare September 4, 2021 17:04

tobiasKaminsky requested a review from AlvaroBrey September 7, 2021 05:21

tobiasKaminsky added the 3. to review label Sep 7, 2021

tobiasKaminsky reviewed Sep 7, 2021

View reviewed changes

src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.java Outdated Show resolved Hide resolved

tobiasKaminsky reviewed Sep 7, 2021

View reviewed changes

AlvaroBrey closed this Sep 7, 2021

AlvaroBrey reopened this Sep 7, 2021

AlvaroBrey reviewed Sep 7, 2021

View reviewed changes

minfaer force-pushed the FixAssembleTimeout branch from 3d9ea97 to 60988c0 Compare September 7, 2021 10:16

minfaer and others added 2 commits September 7, 2021 12:45

Fix function name lowercase

9cd7864

Co-authored-by: Tobias Kaminsky <tobias@nextcloud.com> Signed-off-by: M. Thees <m.thees@intempestatem.eu>

Various coding style improvements

415acf0

Signed-off-by: M. Thees <m.thees@intempestatem.eu>

minfaer force-pushed the FixAssembleTimeout branch from 60988c0 to 415acf0 Compare September 7, 2021 10:50

szaimen mentioned this pull request Sep 10, 2021

Nextcloud fails to register large autouploads as uploaded nextcloud/android#8134

Closed

Add tests

e1c7695

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

tobiasKaminsky requested review from AlvaroBrey and tobiasKaminsky October 18, 2021 07:08

tobiasKaminsky approved these changes Oct 18, 2021

View reviewed changes

AlvaroBrey approved these changes Oct 18, 2021

View reviewed changes

AlvaroBrey merged commit 3d0834d into nextcloud:master Oct 18, 2021

AlvaroBrey added this to the NC Android lib 2.8.0 milestone Oct 18, 2021

hbh7 mentioned this pull request Oct 25, 2021

Upload large mp4 finished with error message nextcloud/android#6405

Closed

AlvaroBrey mentioned this pull request Oct 28, 2021

Auto Upload of large files with server encryption enabled timeout nextcloud/android#9052

Closed

This was referenced Mar 20, 2024

Timeout when assembling large file uploads #696

Closed

Timeouts while assembling chunks on external storage nextcloud/android#12665

Closed

	int AssembleReadTimeout = Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );
	return Math.max(thirtySeconds , Math.min((int)(threeMinutes * file.length() / 1e9) , thirtyMinutes) );

Scale read-timeout on chunk-upload file assembly #709

Scale read-timeout on chunk-upload file assembly #709

Uh oh!

Conversation

minfaer commented Sep 4, 2021 • edited by szaimen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tobiasKaminsky Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlvaroBrey Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

tobiasKaminsky commented Sep 7, 2021

Uh oh!

AlvaroBrey Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

minfaer Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlvaroBrey Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlvaroBrey commented Sep 7, 2021

Uh oh!

minfaer commented Sep 7, 2021

Uh oh!

codecov bot commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

m-terlinde commented Sep 30, 2021

Uh oh!

tobiasKaminsky commented Oct 18, 2021

Uh oh!

nextcloud-android-bot commented Oct 18, 2021

Lint

SpotBugs (new)

SpotBugs (master)

Uh oh!

nextcloud-android-bot commented Oct 18, 2021

Uh oh!

AlvaroBrey commented Oct 18, 2021

Uh oh!

szaimen commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

minfaer commented Sep 4, 2021 •

edited by szaimen

Loading

codecov bot commented Sep 7, 2021 •

edited

Loading