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

Avoid checking for walled connection until necessary #10448

Merged
merged 4 commits into from
Jul 5, 2022

Conversation

AlvaroBrey
Copy link
Member

@AlvaroBrey AlvaroBrey commented Jun 28, 2022

Both when syncing offline files and when retrying failed uploads, don't check for walled internet until the moment it's needed, thus avoiding network calls in cases where it's not needed (missing files, or other conditions not met)

This should reduce load on server, data consumption, and battery drain in such cases.

This is a successor to #10087 with a slightly different approach, thanks @obel1x for your findings and ideas.

In the future we might consider removing this check completely as it is a source of problems, and just handle the errors when trying to do operations in a walled network.

  • Tests written, or not not needed

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #10448 (074b914) into master (074b914) will not change coverage.
The diff coverage is n/a.

❗ Current head 074b914 differs from pull request most recent head c8aa577. Consider uploading reports for the commit c8aa577 to get more accurate results

@@            Coverage Diff            @@
##             master   #10448   +/-   ##
=========================================
  Coverage     31.32%   31.32%           
  Complexity     3235     3235           
=========================================
  Files           536      536           
  Lines         39712    39712           
  Branches       5477     5477           
=========================================
  Hits          12438    12438           
  Misses        25430    25430           
  Partials       1844     1844           

@obel1x
Copy link
Contributor

obel1x commented Jun 28, 2022

hey, i appreachiate that work and vote this up (untestet).

the battery drain is mostly not caused by changed files, but especially in the case, where e.g. autouploaded pictures have been deleted before syncing them. they will get state "file not found" in uploads and the syncworker checked the internetwalled for each file. as the error will persist this will happen for each deleted picture every sync until the upload queue has been cleared.
I did not see a solution for those files as it also would not be ok to not try to upload files. This is, why i removed internet walled completely and since then i am fine with that app.

Does your pull consider these deleted files to not cause pings any more?

@AlvaroBrey
Copy link
Member Author

Does your pull consider these deleted files to not cause pings any more?

Yes, FILE_NOT_FOUND is handled separately and will not cause a ping.

@AlvaroBrey
Copy link
Member Author

Does your pull consider these deleted files to not cause pings any more?

Yes, FILE_NOT_FOUND is handled separately and will not cause a ping.

Actually, not that sure, because this PR only changes things offline sync, not for autoupload; that should be changed separately

@AlvaroBrey
Copy link
Member Author

Does your pull consider these deleted files to not cause pings any more?

Yes, FILE_NOT_FOUND is handled separately and will not cause a ping.

Actually, not that sure, because this PR only changes things offline sync, not for autoupload; that should be changed separately

Handled in c9be796. Will reword this PR to make this explicit.

@AlvaroBrey AlvaroBrey changed the title OfflineSyncWork: avoid checking network connection until necessary Avoid checking for walled connection until necessary Jun 28, 2022
@obel1x
Copy link
Contributor

obel1x commented Jun 28, 2022

tested it: made a picture in offline-mode. picture folder is set to upload only if wlan is free/available. Got connection error shown as message and in upload queue.
Maybe something not working with that setting now? imo it should not try to upload and not giving errors.

@AlvaroBrey could you check to see if this error affects your setup too?

@AlvaroBrey
Copy link
Member Author

tested it: made a picture in offline-mode. picture folder is set to upload only if wlan is free/available. Got connection error shown as message and in upload queue. Maybe something not working with that setting now? imo it should not try to upload and not giving errors.

That is also the behaviour in master, so it's not something introduced in this PR

@tobiasKaminsky
Copy link
Member

/rebase

AlvaroBrey and others added 4 commits July 5, 2022 14:52
…ss sync fails

Co-authored-by: obel1x <obel1x@web.de>
Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
…ssible moment

This avoids network calls if the uploads can't be done for other reason (such as missing files)

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
@tobiasKaminsky tobiasKaminsky force-pushed the fix/offline-sync-server-ping branch from c9be796 to c8aa577 Compare July 5, 2022 12:52
@github-actions
Copy link

github-actions bot commented Jul 5, 2022

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/10448.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@github-actions
Copy link

github-actions bot commented Jul 5, 2022

Codacy

Lint

TypemasterPR
Warnings8585
Errors00

SpotBugs

CategoryBaseNew
Bad practice2828
Correctness4646
Dodgy code356356
Experimental11
Internationalization99
Multithreaded correctness99
Performance6363
Security2828
Total540540

@AlvaroBrey AlvaroBrey merged commit 8cc3bd1 into master Jul 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/offline-sync-server-ping branch July 5, 2022 14:44
@AlvaroBrey AlvaroBrey added this to the Nextcloud App 3.21.0 milestone Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants