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

Generalize Syncer #896

Closed
4 of 6 tasks
rfc2822 opened this issue Jul 11, 2024 · 4 comments · Fixed by #907
Closed
4 of 6 tasks

Generalize Syncer #896

rfc2822 opened this issue Jul 11, 2024 · 4 comments · Fixed by #907
Assignees
Labels
refactoring Internal improvement of existing functions

Comments

@rfc2822
Copy link
Member

rfc2822 commented Jul 11, 2024

Currently: 4 Syncers with duplicated code

Goal:

Important: The current Syncer doesn't close the acquired (address book) content provider! (Fixed in #910, but remember to close it when moving to syncer implementations)

 D  [util.BroadcastReceiverFlowKt] Unregistering broadcast receiver for Action: "android.os.action.POWER_SAVE_MODE_CHANGED"
 W  ApkAssets: Deleting an ApkAssets object '<empty> and /data/data/at.bitfire.davdroid/code_cache/.overlay/base.apk/' with 3 weak references
 D  StrictMode policy violation: android.os.strictmode.LeakedClosableViolation: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
    	at android.os.StrictMode$AndroidCloseGuardReporter.report(StrictMode.java:2007)
    	at dalvik.system.CloseGuard.warnIfOpen(CloseGuard.java:336)
    	at android.content.ContentProviderClient.finalize(ContentProviderClient.java:651)
    	at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:370)
    	at java.lang.Daemons$FinalizerDaemon.processReference(Daemons.java:350)
    	at java.lang.Daemons$FinalizerDaemon.runInternal(Daemons.java:322)
    	at java.lang.Daemons$Daemon.run(Daemons.java:131)
    	at java.lang.Thread.run(Thread.java:1012)
    Caused by: java.lang.Throwable: Explicit termination method 'ContentProviderClient.close' not called
    	at dalvik.system.CloseGuard.openWithCallSite(CloseGuard.java:288)
    	at dalvik.system.CloseGuard.open(CloseGuard.java:257)
    	at android.content.ContentProviderClient.<init>(ContentProviderClient.java:112)
    	at android.content.ContentResolver.acquireContentProviderClient(ContentResolver.java:2594)
    	at at.bitfire.davdroid.sync.Syncer.onPerformSync(Syncer.kt:85)
    	at at.bitfire.davdroid.sync.worker.BaseSyncWorker$doSyncWork$2.invokeSuspend$lambda$0(BaseSyncWorker.kt:297)
    	at at.bitfire.davdroid.sync.worker.BaseSyncWorker$doSyncWork$2.$r8$lambda$iPlAcxGWBi9BGTp3aIafzFPxuss(Unknown Source:0)

Depends on #875

Copy link

This PR/issue depends on:

@rfc2822
Copy link
Member Author

rfc2822 commented Jul 13, 2024

@sunkup I have noticed that the current Syncer from #881 doesn't actually synchronize after the first sync.

Steps to reproduce:

  1. Create DAVx5, set up account, enable a calendar.
  2. Sync.
  3. Watch logs (if you want, also server logs to be sure): CalendarSyncer creates the calendar and syncs it as expected.
  4. Sync again.
  5. Watch logs (if you want, also server logs to be sure): CalendarSyncer runs, but doesn't actually synchronize the collection.

This is quite bad because it prevents testing / reproducing other problems with DAVx5 from the main branch.

Do you think this issue should be addressed in a separate issue/PR before this one (and then use this one only for refactoring) or shall we just do it all at once?

@rfc2822 rfc2822 added the bug Something isn't working label Jul 13, 2024
@sunkup sunkup linked a pull request Jul 13, 2024 that will close this issue
4 tasks
@sunkup
Copy link
Member

sunkup commented Jul 14, 2024

@sunkup I have noticed that the current Syncer from #881 doesn't actually synchronize after the first sync.

Oh, quite bad indeed.

Do you think this issue should be addressed in a separate issue/PR before this one (and then use this one only for refactoring) or shall we just do it all at once?

I have a hunch on why it happens, I think we should address it in a separate issue and PR though. I will create one and work on it straight away :)

@sunkup sunkup removed the bug Something isn't working label Jul 14, 2024
@sunkup
Copy link
Member

sunkup commented Jul 14, 2024

Tracked in #909 Fixed in #910

The PR #910 will also close the acquired provider in the abstract syncer.

The changes should be easy enough to merge into this PR, or simply be re-created accordingly :)

@sunkup sunkup moved this from Todo to In Progress in DAVx⁵ Releases Jul 14, 2024
@sunkup sunkup moved this from In Progress to In Review in DAVx⁵ Releases Jul 30, 2024
@github-project-automation github-project-automation bot moved this from In Review to Done in DAVx⁵ Releases Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants