-
Notifications
You must be signed in to change notification settings - Fork 155
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
Fix crash about several DataStores using the same file #2312
Changes from 4 commits
6eee279
0963aae
0aaa668
9e4feab
b731e2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix 'There are multiple DataStores active for the same file' crashes | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Copyright (c) 2024 New Vector Ltd | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.element.android.libraries.di.annotations | ||
|
||
import javax.inject.Qualifier | ||
|
||
/** | ||
* Qualifies a [CoroutineScope] object which represents the base coroutine scope to use for an active session. | ||
*/ | ||
@Retention(AnnotationRetention.RUNTIME) | ||
@MustBeDocumented | ||
@Qualifier | ||
annotation class SessionCoroutineScope |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* | ||
* Copyright (c) 2024 New Vector Ltd | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.element.android.libraries.preferences.impl.store | ||
|
||
import android.content.Context | ||
import io.element.android.libraries.di.AppScope | ||
import io.element.android.libraries.di.ApplicationContext | ||
import io.element.android.libraries.di.SingleIn | ||
import io.element.android.libraries.matrix.api.core.SessionId | ||
import kotlinx.coroutines.CoroutineScope | ||
import java.util.concurrent.ConcurrentHashMap | ||
import javax.inject.Inject | ||
|
||
@SingleIn(AppScope::class) | ||
Check warning on line 28 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L28
|
||
class DefaultSessionPreferencesStoreFactory @Inject constructor( | ||
@ApplicationContext private val context: Context, | ||
Check warning on line 30 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L30
|
||
) { | ||
private val cache = ConcurrentHashMap<SessionId, DefaultSessionPreferencesStore>() | ||
Check warning on line 32 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L32
|
||
|
||
fun get(sessionId: SessionId, sessionCoroutineScope: CoroutineScope) = cache.getOrPut(sessionId) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a warning here: |
||
DefaultSessionPreferencesStore(context, sessionId, sessionCoroutineScope) | ||
Check warning on line 35 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L34-L35
|
||
} | ||
|
||
fun remove(sessionId: SessionId) = cache.remove(sessionId) | ||
Check warning on line 38 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L38
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I might just remove this and |
||
|
||
fun clear() = cache.clear() | ||
Check warning on line 40 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/DefaultSessionPreferencesStoreFactory.kt#L40
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used, but I do not see in which case we may want to have such method. So maybe just remove it? |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Copyright (c) 2024 New Vector Ltd | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.element.android.libraries.preferences.impl.store | ||
|
||
import com.squareup.anvil.annotations.ContributesTo | ||
import dagger.Module | ||
import dagger.Provides | ||
import io.element.android.features.preferences.api.store.SessionPreferencesStore | ||
import io.element.android.libraries.di.SessionScope | ||
import io.element.android.libraries.di.SingleIn | ||
import io.element.android.libraries.di.annotations.SessionCoroutineScope | ||
import io.element.android.libraries.matrix.api.user.CurrentSessionIdHolder | ||
import kotlinx.coroutines.CoroutineScope | ||
|
||
@Module | ||
@ContributesTo(SessionScope::class) | ||
object SessionPreferencesModule { | ||
@Provides | ||
@SingleIn(SessionScope::class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, I was just keeping it there in case we can solve the underlying issue and remove |
||
fun providesSessionPreferencesStore( | ||
defaultSessionPreferencesStoreFactory: DefaultSessionPreferencesStoreFactory, | ||
currentSessionIdHolder: CurrentSessionIdHolder, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this object has the scope Oh actually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The problem is Anvil/Dagger doesn't understand that a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We had annotations to be able to inject |
||
@SessionCoroutineScope sessionCoroutineScope: CoroutineScope, | ||
): SessionPreferencesStore { | ||
return defaultSessionPreferencesStoreFactory | ||
.get(currentSessionIdHolder.current, sessionCoroutineScope) | ||
Check warning on line 40 in libraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/SessionPreferencesModule.kt Codecov / codecov/patchlibraries/preferences/impl/src/main/kotlin/io/element/android/libraries/preferences/impl/store/SessionPreferencesModule.kt#L39-L40
|
||
} | ||
} |
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.
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.