fix(common): Resolve thread deadlock in HeartBeatInfoStorage#8182
fix(common): Resolve thread deadlock in HeartBeatInfoStorage#8182milaGGL wants to merge 8 commits into
Conversation
Remove the legacy `synchronized` keyword from private helper methods and pure utility methods inside `HeartBeatInfoStorage` (such as `getStoredUserAgentString`, `updateStoredUserAgent`, `removeStoredDate`, `getFormattedDate`, `cleanUpStoredHeartBeats`, and `isSameDateUtc`). Under the bug, `storeHeartBeat()` locked the class monitor while blocking in `runBlocking` for Jetpack DataStore's `editSync()` to complete. Because DataStore schedules transactions on a background thread, helper methods called inside the transaction block that were also synchronized on the same monitor resulted in a permanent deadlock. Since these helper methods operate exclusively on thread-local transaction parameters and do not access shared mutable instance state, they do not require synchronization. Add regression unit test `storeHeartBeat_whenCalledOnSeparateThread_doesNotDeadlock` to verify the fix. Fixes #8016
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
| assertThat(heartBeatDataStore.getSync(GLOBAL, -1L)).isEqualTo(currentTime); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Move this tests to a kolin file so there's no need to use language bridging features, like kotlin.jvm.functions.Function1
There was a problem hiding this comment.
create a kotlin file to put the test case in
…ub.com/firebase/firebase-android-sdk into mila/fix-HeartBeatInfoStorage-deadlock
2 participants
The Bug
The deadlock was caused by mixing legacy Java synchronized monitor locks with modern asynchronous Kotlin Coroutines inside the JavaDataStorage wrapper.
storeHeartBeat(...)method, acquiring the monitor lock onHeartBeatInfoStorage.this. It then blocks insiderunBlockingwaiting for the Jetpack DataStore preference transaction (JavaDataStorage.editSync(...)) to complete.getStoredUserAgentString,updateStoredUserAgent,cleanUpStoredHeartBeats). Because these helper methods were also declared as synchronized, Thread B attempts to acquire the monitor lock onHeartBeatInfoStorage.this, causing a permanent thread deadlock.The Fix
Removed the synchronized keyword from all private helper methods and utility methods.
These methods operate exclusively on thread-local transaction parameters and do not access shared mutable instance state, making synchronization unnecessary. This allows the background thread to execute them safely without acquiring the class monitor.
Verification
Added the regression unit test
storeHeartBeat_whenCalledOnSeparateThread_doesNotDeadlockto reproduce the deadlock condition and verify the fix.Fixes #8016