Skip to content

Reset last sync date of files written via DocumentsProvider#6751

Merged
tobiasKaminsky merged 1 commit into
nextcloud:masterfrom
grote:6726-documentsprovider-0-bytes
Aug 24, 2020
Merged

Reset last sync date of files written via DocumentsProvider#6751
tobiasKaminsky merged 1 commit into
nextcloud:masterfrom
grote:6726-documentsprovider-0-bytes

Conversation

@grote

@grote grote commented Aug 19, 2020

Copy link
Copy Markdown
Member

The sync operation detects changes by comparing timestamps in milliseconds. However, the local modification time only has a precision of seconds (even though given in milliseconds).
When the DocumentsProvider is used programmatically, operations such as a create and a subsequent write can happen within the same second causing the sync operation to not detect the change and therefore failing to upload a file causing data loss.

This MR also moves the file close listener to the thread used by the DocumentsStorageProvider as this is more appropriate than the UI thread.

Fixes #6726 (I can create other tickets about the side-issues identified there)

@tobiasKaminsky

Copy link
Copy Markdown
Member

The sync operation detects changes by comparing timestamps in milliseconds. However, the local modification time only has a precision of seconds (even though given in milliseconds).

While reviewing this I tried to find where we use precision of seconds, as we then should change this, but I cannot find it?
OCfile:getLocalModificationTimestamp() uses File.lastModified which is in millis.

@grote

grote commented Aug 20, 2020

Copy link
Copy Markdown
Member Author

While reviewing this I tried to find where we use precision of seconds, as we then should change this, but I cannot find it?
OCfile:getLocalModificationTimestamp() uses File.lastModified which is in millis.

Sorry for not explaining this better. The precision used is actually not obvious and I only found out about it with a long debugging session pinning down the bug that was probably introduced because millsecond precision is assumed as the java docs talk about the time being in milliseconds. Eventually I noticed that the last three digits of the getLocalModificationTimestamp() were always 0. I did many test runs and they always stayed at zero.

Here's a related SO question: https://stackoverflow.com/questions/24804618/get-file-mtime-with-millisecond-resolution-from-java
Somebody claims that this is fixed in Java 9, but on Android 10 (which should use Java 9 internally) I also get second precision only.

Note that this PR is superfluous once #6753 is implemented. However, since we don't know when this will land, it might make sense to merge this one already since it fixes a rather serious data-loss bug.

@tobiasKaminsky

Copy link
Copy Markdown
Member

Thanks for your detailed explanation!
So we cannot do anything on our side, as we are already ready for millis :-)

Then let us get this in 👍

Comment thread src/main/java/com/owncloud/android/providers/DocumentsStorageProvider.java Outdated
The sync operation detects changes by comparing timestamps in
milliseconds. However, the local modification time only has a precision
of seconds (even though given in milliseconds).
When the DocumentsProvider is used programmatically, operations such as
a create and a subsequent write can happen within the same second
causing the sync operation to not detect the change and therefore
failing to upload a file causing data loss.

This commit also moves the file close listener to the thread used by the
DocumentsStorageProvider as this is more appropriate than the UI thread.

Fixes nextcloud#6726

Signed-off-by: Torsten Grote <t@grobox.de>
@grote grote force-pushed the 6726-documentsprovider-0-bytes branch from f23010c to 64ec705 Compare August 21, 2020 12:15
@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/15527.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.

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Codacy

405

Lint

TypemasterPR
Warnings8181
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings61
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings44
Dodgy code Warnings106
Total329

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings61
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings44
Dodgy code Warnings106
Total329

@codecov

codecov Bot commented Aug 21, 2020

Copy link
Copy Markdown

Codecov Report

Merging #6751 into master will increase coverage by 0.07%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #6751      +/-   ##
============================================
+ Coverage     27.81%   27.88%   +0.07%     
  Complexity        5        5              
============================================
  Files           427      427              
  Lines         34237    34239       +2     
  Branches       4735     4735              
============================================
+ Hits           9522     9549      +27     
+ Misses        23297    23260      -37     
- Partials       1418     1430      +12     
Impacted Files Coverage Δ Complexity Δ
...ud/android/providers/DocumentsStorageProvider.java 3.08% <0.00%> (-0.02%) 0.00 <0.00> (ø)
.../third_parties/daveKoeller/AlphanumComparator.java 82.14% <0.00%> (-1.20%) 0.00% <0.00%> (ø%)
...oud/android/operations/RefreshFolderOperation.java 70.99% <0.00%> (-1.15%) 0.00% <0.00%> (ø%)
...oud/android/ui/activity/SyncedFoldersActivity.java 24.16% <0.00%> (ø) 0.00% <0.00%> (ø%)
...loud/android/datamodel/FileDataStorageManager.java 52.58% <0.00%> (+0.31%) 0.00% <0.00%> (ø%)
...loud/android/datamodel/ThumbnailsCacheManager.java 38.90% <0.00%> (+0.36%) 0.00% <0.00%> (ø%)
...d/android/operations/SynchronizeFileOperation.java 1.05% <0.00%> (+1.05%) 0.00% <0.00%> (ø%)
...va/com/owncloud/android/utils/FilesSyncHelper.java 17.33% <0.00%> (+1.33%) 0.00% <0.00%> (ø%)
.../owncloud/android/files/services/FileUploader.java 50.41% <0.00%> (+1.44%) 0.00% <0.00%> (ø%)
.../java/com/nextcloud/client/jobs/OfflineSyncWork.kt 35.59% <0.00%> (+27.11%) 0.00% <0.00%> (ø%)

@tobiasKaminsky tobiasKaminsky merged commit ad7564b into nextcloud:master Aug 24, 2020
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.14.0 milestone Aug 24, 2020
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.

Files written via DocumentsProvider often 0 bytes

4 participants