[stable10] [revived] Retry chunks in web UI on stalled uploads

[PVince81]

Reverts #31185, bringing back #31005.
Second chance to make it right

Setup instructions for testing here:

• old and wrong results: Retry on stalled upload #30776 (comment)
• patch for DAV endpoint: https://gist.github.com/PVince81/2978ed02a21876fd9e58f74beda7e5c0 (or in the future, use Storage wrapper in testing app to simulate upload timeout #32183)
• occ config:app:set files max_chunk_size --value 1024
• occ config:app:set files upload_stall_timeout --value 3

Fixes #30770

• debug issue => it was the testing code...

[PVince81]

What I observe now here from several tests is that it stays stuck on "Processing files" forever, even for small files.

[PVince81]

the final MOVE doesn't return

[PVince81]

stuck here:

[PVince81]

infinite loop in https://github.com/owncloud/core/blob/revert-31185-revert-31005-stable10-retry-on-stalled-upload/apps/dav/lib/Upload/AssemblyStream.php#L120 because the data returned by fread has zero bytes.

[PVince81]

ok, seems empty chunks are there and are causing infinite loop

• solve infinite loop to skip empty chunks. or maybe we should error in case of empty chunks in https://github.com/owncloud/core/blob/revert-31185-revert-31005-stable10-retry-on-stalled-upload/apps/dav/lib/Upload/AssemblyStream.php#L120
• find out why empty chunks exist in the first place

Edit: see #32170 (comment) for resolution

[PVince81]

ok, so if chunks are zero on disk or uploaded as zero, there is no issue, also not on stable10

however in this scenario here, the chunks have a zero size on disk but the oc_filecache reports their size as being 1024 !

so it expects 1024 bytes but when reading it actually receives 0.

maybe we need to check the size on-disk...

also why do we even have this info in the file cache...

[PVince81]

annnd the reason for all this confusion is because I didn't patch Directory.php correctly and basically leaved the old code with file_put_contents and the code with the random failure hack.

[PVince81]

Another round of testing with the correct setup now.

Whenever a file has no retried chunks, all works fine.

As soon as at least one chunk is retried, the final MOVE tells that the size of chunks doesn't match the expected size.

Looking at the network console in the browser shows that the resuming all worked fine and the sent file contents is correct.
On the server side however, one of the chunks has zero size despite the fact that the client did send it.

I wonder if this is a side effect of the way we simulate the timeout.

[PVince81]

Turns out that the exit() in the simulation patch still seems to return 200 OK with empty response to the client, despite aborting. This makes the JS side believe that the transfer was successful when it wasn't. Ok, so maybe another potential case to handle.

I noticed that chunk uploads have various possible responses: sometimes 201 created, sometimes 204 no content and sometimes 200. The 200 ok is likely wrong here. But I'm not sure whether we should strictly expect 201 or 204, this feels wrong.

Maybe the problem is simply that the simulation code is not accurate. Either we can increase the sleep there to cause a real timeout situation, or make it throw a real exception.

[PVince81]

@DeepDiver1975 @butonic there is no code change since the last review before the revert.

The problem was not in the JS resuming code but in the patch that @butonic supplied for testing, as it was returning 200 OK sometimes due to the exit() call. I improved the patch to simulate actual timeout by setting set_time_limit() and when running this I can see the timeout errors in the log, so it works.

Please review the new testing patch then to confirm that this is enough to confirm that all works.

I ran several tests with small and big files and it was able to resume properly.

[DeepDiver1975]

@ownclouders rebase

[ownclouders]

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

[ownclouders]

Automated rebase with GitMate.io was successful! 🎉

[codecov]

Codecov Report

Merging #32170 into stable10 will decrease coverage by <.01%.
The diff coverage is 0%.

@@              Coverage Diff               @@
##             stable10   #32170      +/-   ##
==============================================
- Coverage       62.78%   62.78%   -0.01%     
  Complexity      18719    18719              
==============================================
  Files            1220     1220              
  Lines           73380    73384       +4     
  Branches         1279     1279              
==============================================
+ Hits            46070    46072       +2     
- Misses          26931    26933       +2     
  Partials          379      379

Flag	Coverage Δ	Complexity Δ
#javascript	53.08% <ø> (+0.02%)	0 <ø> (ø)	⬇️
#phpunit	63.84% <0%> (-0.01%)	18719 <0> (ø)

Impacted Files	Coverage Δ	Complexity Δ
apps/files/lib/App.php	25% <0%> (-12.5%)	3 <0> (ø)
core/js/js.js	54.89% <0%> (+0.23%)	0% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05870fb...7353c36. Read the comment docs.

[DeepDiver1975]

conflict resolved ...