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

Fix unrecoverable freezing when PutMultiFileJob is used with upload rate limits enabled #5680

Merged
merged 16 commits into from
May 15, 2023

Conversation

claucambra
Copy link
Collaborator

@claucambra claucambra commented May 11, 2023

On bulk upload we create a QHttpMultipart object which will take several UploadDevices in. Internally, the QHttpMultipart class has a QIODevice subclass which uses a while loop to read the contents of our UploadDevices.

With rate limits enabled this causes an infinite loop as we are not providing any data in UploadDevice's readData when the device is choked or limited.

This PR fixes the app freeze by disabling rate limits on upload devices used in bulk upload PutMultiFileJobs, though this means no rate limiting on these jobs (though this was never working anyway so no loss here)

This PR also includes a bit of additional logging, and lots of code modernisation fixes. The key commit is e72b68c

Closes #5675
Closes #5094

@claucambra claucambra self-assigned this May 11, 2023
@claucambra claucambra force-pushed the bugfix/putmultifilejob-crash-mac branch from 34771df to 548cfff Compare May 11, 2023 13:02
@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #5680 (49bb5a0) into master (ade138c) will increase coverage by 0.05%.
The diff coverage is 45.76%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5680      +/-   ##
==========================================
+ Coverage   59.19%   59.25%   +0.05%     
==========================================
  Files         143      143              
  Lines       18469    18463       -6     
==========================================
+ Hits        10933    10940       +7     
+ Misses       7536     7523      -13     
Impacted Files Coverage Δ
src/libsync/putmultifilejob.h 0.00% <ø> (-75.00%) ⬇️
src/libsync/bandwidthmanager.cpp 39.39% <3.33%> (+2.07%) ⬆️
src/libsync/putmultifilejob.cpp 91.83% <84.61%> (-8.17%) ⬇️
src/libsync/bulkpropagatorjob.cpp 74.71% <93.75%> (+1.53%) ⬆️

... and 3 files with indirect coverage changes

@claucambra claucambra changed the title Bugfix/putmultifilejob crash Fix unrecoverable freezing when PutMultiFileJob is used with upload rate limits enabled May 15, 2023
…const auto, readability, etc

Signed-off-by: Claudio Cambra <[email protected]>
@claucambra claucambra force-pushed the bugfix/putmultifilejob-crash-mac branch from 0d6d570 to 49bb5a0 Compare May 15, 2023 12:46
@claucambra
Copy link
Collaborator Author

/backport to stable-3.8

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5680-49bb5a005c1411bc118ed5dd949b85ebe1c29c96-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarcloud
Copy link

sonarcloud bot commented May 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Alysko
Copy link

Alysko commented Aug 3, 2023

I still have the problem with 3.9.1. By deleting [BWLimit] in the conf, it works.

@gege2b
Copy link

gege2b commented Jan 11, 2024

Problem still present with 3.11.0 on windows 10
Disabling BW Limit makes it work again

@ohCiep6j
Copy link

Problem still present with 3.11.0 on windows 10 Disabling BW Limit makes it work again

Same on Windows 11, same client version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Crash on macOS [Bug]: Desktop Client freezes in a 100% CPU load loop after server upgrade to 25
6 participants