fix(state): prevent multiple DataStore instances for same file#73
Conversation
Fixes critical bug where defaultDataStore() creates new DataStore instances on every call instead of returning the singleton. This caused fatal IllegalStateException crashes in React Native apps where multiple DataStores were active for rownd_state.json. Root cause: DataStoreFactory.create() result was not assigned to the companion object's dataStore variable, breaking the singleton pattern. Fixes: Multiple DataStores active for the same file exception
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures the Rownd state DataStore is a true singleton by caching the created instance in the StateRepo companion object instead of creating a new DataStore on each access, preventing multiple active DataStores for the same file. Sequence diagram for singleton DataStore access in StateReposequenceDiagram
participant ReactNativeComponent
participant StateRepoCompanion as StateRepo_companion
participant DataStore
ReactNativeComponent->>StateRepoCompanion: defaultDataStore(context)
alt dataStore is null
StateRepoCompanion->>StateRepoCompanion: DataStoreFactory.create(storage, scope, corruptionHandler)
StateRepoCompanion->>StateRepoCompanion: assign to dataStore
end
StateRepoCompanion-->>ReactNativeComponent: return dataStore
ReactNativeComponent->>StateRepoCompanion: defaultDataStore(context) (later call)
StateRepoCompanion->>StateRepoCompanion: check existing dataStore
StateRepoCompanion-->>ReactNativeComponent: return same dataStore instance
Updated class diagram for StateRepo DataStore singletonclassDiagram
class StateRepo {
}
class StateRepo_companion {
-DataStore~GlobalState~ dataStore
+defaultDataStore(context: Context) DataStore~GlobalState~
}
class DataStore~GlobalState~ {
}
StateRepo_companion --> DataStore~GlobalState~ : holds_singleton
StateRepo .. StateRepo_companion : companion_object
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The companion-object singleton initialization still isn’t thread-safe; consider using a synchronized block or
lazyinitialization (or the official DataStore singleton pattern) to avoid races where two threads might both seedataStore == nulland create separate instances.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The companion-object singleton initialization still isn’t thread-safe; consider using a synchronized block or `lazy` initialization (or the official DataStore singleton pattern) to avoid races where two threads might both see `dataStore == null` and create separate instances.
## Individual Comments
### Comment 1
<location path="android/src/main/java/io/rownd/android/models/repos/StateRepo.kt" line_range="210-213" />
<code_context>
}
- return DataStoreFactory.create(
+ dataStore = DataStoreFactory.create(
storage = FileStorage(GlobalStateSerializer) {
context.dataStoreFile(Rownd.config.stateFileName)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid using `dataStore!!` and instead return the non-null value directly.
The old implementation returned `DataStoreFactory.create(...)` directly, which is non-null by construction. Now the result is stored in (presumably nullable) `dataStore` and returned via `dataStore!!`. This introduces an unnecessary `!!` and a potential NPE if the code is later refactored. Instead, return the created value directly and then assign it, e.g.:
```kotlin
val ds = DataStoreFactory.create(
storage = FileStorage(GlobalStateSerializer) {
context.dataStoreFile(Rownd.config.stateFileName)
},
scope = CoroutineScope(Dispatchers.IO + SupervisorJob()),
corruptionHandler = ReplaceFileCorruptionHandler { GlobalState() }
)
dataStore = ds
return ds
```
```suggestion
val ds = DataStoreFactory.create(
storage = FileStorage(GlobalStateSerializer) {
context.dataStoreFile(Rownd.config.stateFileName)
},
return@ReplaceFileCorruptionHandler GlobalState()
}
)
dataStore = ds
return ds
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
- Add @synchronized annotation to prevent race conditions where multiple threads could create separate DataStore instances - Store result in local variable and return directly to avoid !! assertion - Use applicationContext to prevent memory leaks from short-lived contexts Addresses Sourcery review feedback on thread safety and best practices.
2 participants
Issue
Fixes critical bug causing fatal crashes in React Native applications with error:
Root Cause
The
defaultDataStore()companion function inStateRepo.kthad a broken singleton pattern. It checked if a DataStore instance existed, but when creating a new one, it failed to assign the result to the companion object'sdataStorevariable.Before:
After:
Impact
Testing
Fixes multiple DataStore collision errors reported by customers.
Summary by Sourcery
Bug Fixes: