Fix crash about several DataStores using the same file #2312

jmartinesp · 2024-01-29T09:46:15Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Fix crash about several DataStores using the same file
- Create @SessionCoroutineScope annotation to pass a session-managed coroutine scope to the DI.
- Expose this scope from MatrixClient.
- Rework SessionPreferencesStore file creation a bit. Use this coroutine scope to initialize the data store, so it's stopped when the scope is cancelled.
Make sure the whole item in advanced settings screen triggers the toggle action, to be consistent with the rest of the app.

Motivation and context

Fixes #2308 (I hope).

Tests

To be honest, I'm not 100% sure how to trigger this issue. I tried logging out and in again, which was my theory for why this was happening, but sadly I couldn't reproduce the issue.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

- Create `@SessionCoroutineScope` annotation to pass a session-managed coroutine scope to the DI. - Expose this scope from `MatrixClient`. - Rework DataStore file creation a bit.

github-actions · 2024-01-29T09:53:26Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/MTvZYk

codecov · 2024-01-29T11:37:06Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (5b0268a) 70.08% compared to head (b731e2e) 70.06%.
Report is 13 commits behind head on develop.

Files	Patch %	Lines
...ences/impl/store/DefaultSessionPreferencesStore.kt	0.00%	5 Missing ⚠️
...mpl/store/DefaultSessionPreferencesStoreFactory.kt	0.00%	5 Missing ⚠️
...preferences/impl/store/SessionPreferencesModule.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2312      +/-   ##
===========================================
- Coverage    70.08%   70.06%   -0.02%     
===========================================
  Files         1353     1355       +2     
  Lines        33247    33256       +9     
  Branches      6877     6877              
===========================================
+ Hits         23301    23302       +1     
- Misses        6638     6646       +8     
  Partials      3308     3308

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ganfra

One remark otherwise LGTM

ganfra · 2024-01-29T12:37:26Z

app/src/main/kotlin/io/element/android/x/di/SessionComponent.kt

@@ -33,6 +35,10 @@ interface SessionComponent : NodeFactoriesBindings {
    interface Builder {
        @BindsInstance
        fun client(matrixClient: MatrixClient): Builder
+
+        @BindsInstance


Why using @BindsInstance here? I'd just use @Provides

Won't @Provides need the function to be implemented?

We already have lots of @provides in the SessionMatrixModule. I'd use the builder only for things we got from the outer world.

Oh, now I see what you mean. Yeah, a provides is better here.

bmarty · 2024-01-29T16:42:38Z

@jmartinesp I can repro the crash on the latest nightly systematically by:

opening the app
open a timeline
clicking back twice to show the device home
opening a room timeline from a notification.

But they are other recipes, for instance:

open a room
click back twice
open the app again
sometimes it crashes the app.

bmarty · 2024-01-29T16:43:57Z

changelog.d/2308.bugfix

@@ -0,0 +1 @@
+Fix 'There are multiple DataStores active for the same file' crashes


I am tempted to say that it's not necessary to add a file for the changelog, since the crash was not on the previous release.

jmartinesp · 2024-01-29T17:16:28Z

@bmarty thanks for the reproducible steps! Sadly, it seems like the fix does not really fix the issue then... I'll keep trying.

…ncesStoreFactory` until we figure out what went wrong with the scoping

bmarty

Glad to see that the issue is fixed!
I have some more remarks before we can merge this PR.

bmarty · 2024-01-30T08:08:26Z

.../main/kotlin/io/element/android/libraries/preferences/impl/store/SessionPreferencesModule.kt

+@ContributesTo(SessionScope::class)
+object SessionPreferencesModule {
+    @Provides
+    @SingleIn(SessionScope::class)


I think @SingleIn(SessionScope::class) is not necessary here. defaultSessionPreferencesStoreFactory is already handling the concurrency access.

True, I was just keeping it there in case we can solve the underlying issue and remove DefaultSessionPreferencesStoreFactory. But it doesn't do anything at the moment, so we can remove it.

bmarty · 2024-01-30T08:09:13Z

.../main/kotlin/io/element/android/libraries/preferences/impl/store/SessionPreferencesModule.kt

+    @SingleIn(SessionScope::class)
+    fun providesSessionPreferencesStore(
+        defaultSessionPreferencesStoreFactory: DefaultSessionPreferencesStoreFactory,
+        currentSessionIdHolder: CurrentSessionIdHolder,


Since this object has the scope SessionScope, can we inject directly the sessionId: String instead of the CurrentSessionIdHolder?

Oh actually CurrentSessionIdHolder is not what I though. It's not really a Holder, but more an accessor (or a provider). I think it will be simpler to provide the sessionId in SessionMatrixModule like some other fields and remove CurrentSessionIdHolder. Out of scope of this PR though.

I think it will be simpler to provide the sessionId in SessionMatrixModule like some other fields and remove CurrentSessionIdHolder. Out of scope of this PR though.

The problem is Anvil/Dagger doesn't understand that a SessionId is since it's Java based, for the DI library it's just a String and it won't allow you to inject it anywhere. That's why we had to create the holder wrapper. But maybe its name can be misleading.

We had annotations to be able to inject Strings in the previous project and it was quite handy.

bmarty · 2024-01-30T08:15:16Z

...io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt

+) {
+    private val cache = ConcurrentHashMap<SessionId, DefaultSessionPreferencesStore>()
+
+    fun get(sessionId: SessionId, sessionCoroutineScope: CoroutineScope) = cache.getOrPut(sessionId) {


There is a warning here:
Declaration has type inferred from a platform call, which can lead to unchecked nullability issues. Specify type explicitly as nullable or non-nullable.

bmarty · 2024-01-30T08:15:27Z

...io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt

+        DefaultSessionPreferencesStore(context, sessionId, sessionCoroutineScope)
+    }
+
+    fun remove(sessionId: SessionId) = cache.remove(sessionId)


Yeah, I might just remove this and clear until I create a way to clear these values in another PR (#2303 ?).

bmarty · 2024-01-30T08:16:03Z

...io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt

+
+    fun remove(sessionId: SessionId) = cache.remove(sessionId)
+
+    fun clear() = cache.clear()


Not used, but I do not see in which case we may want to have such method. So maybe just remove it?

sonarcloud · 2024-01-30T09:20:09Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

bmarty

Thanks for the update!

Fix crash about several DataStores using the same file

6eee279

- Create `@SessionCoroutineScope` annotation to pass a session-managed coroutine scope to the DI. - Expose this scope from `MatrixClient`. - Rework DataStore file creation a bit.

jmartinesp requested a review from ganfra January 29, 2024 09:46

jmartinesp requested a review from a team as a code owner January 29, 2024 09:46

jmartinesp force-pushed the fix/jme/multiple-data-stores-crash branch from 0a54f82 to bc47708 Compare January 29, 2024 10:43

Add changelog

0963aae

jmartinesp force-pushed the fix/jme/multiple-data-stores-crash branch from bc47708 to 0963aae Compare January 29, 2024 11:12

ganfra approved these changes Jan 29, 2024

View reviewed changes

Use @Provides for the @SessionCoroutineScope instance

0aaa668

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 29, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 29, 2024

bmarty reviewed Jan 29, 2024

View reviewed changes

Centralise session preference creation through `DefaultSessionPrefere…

9e4feab

…ncesStoreFactory` until we figure out what went wrong with the scoping

bmarty reviewed Jan 30, 2024

View reviewed changes

Fix review issues

b731e2e

bmarty approved these changes Jan 30, 2024

View reviewed changes

jmartinesp added the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 30, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label Jan 30, 2024

jmartinesp merged commit ede1dc0 into develop Jan 30, 2024
16 of 18 checks passed

jmartinesp deleted the fix/jme/multiple-data-stores-crash branch January 30, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash about several DataStores using the same file #2312

Fix crash about several DataStores using the same file #2312

jmartinesp commented Jan 29, 2024

github-actions bot commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

ganfra left a comment

ganfra Jan 29, 2024

jmartinesp Jan 29, 2024

ganfra Jan 29, 2024

jmartinesp Jan 29, 2024

bmarty commented Jan 29, 2024 •

edited

Loading

bmarty Jan 29, 2024

jmartinesp commented Jan 29, 2024

bmarty left a comment

bmarty Jan 30, 2024

jmartinesp Jan 30, 2024

bmarty Jan 30, 2024

jmartinesp Jan 30, 2024

bmarty Jan 30, 2024

bmarty Jan 30, 2024

bmarty Jan 30, 2024

jmartinesp Jan 30, 2024

bmarty Jan 30, 2024

sonarcloud bot commented Jan 30, 2024

bmarty left a comment

		@@ -0,0 +1 @@
		Fix 'There are multiple DataStores active for the same file' crashes


		fun remove(sessionId: SessionId) = cache.remove(sessionId)

		fun clear() = cache.clear()

Fix crash about several DataStores using the same file #2312

Fix crash about several DataStores using the same file #2312

Conversation

jmartinesp commented Jan 29, 2024

Type of change

Content

Motivation and context

Tests

Tested devices

Checklist

github-actions bot commented Jan 29, 2024 • edited Loading

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

ganfra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

jmartinesp commented Jan 29, 2024

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 30, 2024

Quality Gate passed

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading

bmarty commented Jan 29, 2024 •

edited

Loading