-
Notifications
You must be signed in to change notification settings - Fork 302
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
Synchronize sync workers #2385
Synchronize sync workers #2385
Conversation
1f0b113
to
d204682
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to check the change.
I am not sure if automation test for this is a good idea. Because it takes lot of execution time to be able to verify the synchronicity of any concurrent jobs. See how concurrent behavior is verified here. Such long execution time, however, is very likely in case of long sync time in an actual app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please feel free to merge once tested manually again.
engine/src/main/java/com/google/android/fhir/sync/FhirSyncWorker.kt
Outdated
Show resolved
Hide resolved
I figured how to test the mutex locking. |
engine/src/test/java/com/google/android/fhir/sync/FhirSynchronizerTest.kt
Outdated
Show resolved
Hide resolved
engine/src/test/java/com/google/android/fhir/sync/FhirSynchronizerTest.kt
Outdated
Show resolved
Hide resolved
* mutex is enough * Mutex is actually good * Remove extra mutex * remove extra spotless changes * spotless * mutex test added * adding more asserts * test modified * note for flaky test possibility
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2361
Description
We use the recommended way for mutual exclusion: link
Solution: Wrap the
FhirSynchronizer#synchronize
withmutex.withLock
mutex.withLock will suspend the successive invocations of
FhirSynchronizer#synchronize
, without blocking the thread, until the lock is released. Hence all work requests will be processed - and in FIFO order.Alternative(s) considered
Alternative: One alternative involves using a ReentrantLock within a try-catch-finally block. This approach essentially replicates mutex-like behavior to manage concurrent access to shared resources. It would require maintaining a job queue to handle work requests sequentially. Without a queue, workers would need to return Result.retry, leading to repeated attempts every 30 seconds.
Another alternative was to wrap
FhirSyncWorker#doWork
with mutex.withLock but based on this comment and also for the fact that FhirSynchronizer#synchronize should run sequentially irrespective of whether FhirSyncWorker uses it or not.Type
Bug fix
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.