From 0632d01d866624f17442c6cdbcea0926f207bbb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Mon, 17 Jul 2023 08:05:12 +0200 Subject: [PATCH 1/8] Fix sliding sync loop restarts due to expirations Both `NotifiableEventResolver` and `DefaultNotificationDrawerManager` were creating new Rust SDK Clients while processing notifications instead of reusing the already existing one. --- .../io/element/android/appnav/RootFlowNode.kt | 2 +- .../android/appnav/di/MatrixClientsHolder.kt | 66 -------------- changelog.d/880.bugfix | 1 + .../matrix/ui/di/MatrixClientsHolder.kt | 87 +++++++++++++++++++ .../DefaultNotificationDrawerManager.kt | 4 +- .../notifications/NotifiableEventResolver.kt | 6 +- 6 files changed, 96 insertions(+), 70 deletions(-) create mode 100644 changelog.d/880.bugfix create mode 100644 libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index 4150bcaebc..71f80dae3e 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -37,7 +37,7 @@ import com.bumble.appyx.navmodel.backstack.operation.push import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode -import io.element.android.appnav.di.MatrixClientsHolder +import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder import io.element.android.appnav.intent.IntentResolver import io.element.android.appnav.intent.ResolvedIntent import io.element.android.appnav.root.RootPresenter diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index bbb14d4d29..53a49aef9f 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -16,69 +16,3 @@ package io.element.android.appnav.di -import com.bumble.appyx.core.state.MutableSavedStateMap -import com.bumble.appyx.core.state.SavedStateMap -import io.element.android.libraries.matrix.api.MatrixClient -import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService -import io.element.android.libraries.matrix.api.core.SessionId -import kotlinx.coroutines.runBlocking -import timber.log.Timber -import java.util.concurrent.ConcurrentHashMap -import javax.inject.Inject - -private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" - -class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { - - private val sessionIdsToMatrixClient = ConcurrentHashMap() - - fun add(matrixClient: MatrixClient) { - sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient - } - - fun removeAll() { - sessionIdsToMatrixClient.clear() - } - - fun remove(sessionId: SessionId) { - sessionIdsToMatrixClient.remove(sessionId) - } - - fun isEmpty(): Boolean = sessionIdsToMatrixClient.isEmpty() - - fun knowSession(sessionId: SessionId): Boolean = sessionIdsToMatrixClient.containsKey(sessionId) - - fun getOrNull(sessionId: SessionId): MatrixClient? { - return sessionIdsToMatrixClient[sessionId] - } - - @Suppress("UNCHECKED_CAST") - fun restore(state: SavedStateMap?) { - Timber.d("Restore state") - if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { - Timber.w("Restore with non-empty map") - } - val sessionIds = state[SAVE_INSTANCE_KEY] as? Array - Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") - if (sessionIds.isNullOrEmpty()) return - // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. - runBlocking { - sessionIds.forEach { sessionId -> - Timber.d("Restore matrix session: $sessionId") - authenticationService.restoreSession(sessionId) - .onSuccess { matrixClient -> - add(matrixClient) - } - .onFailure { - Timber.e("Fail to restore session") - } - } - } - } - - fun save(state: MutableSavedStateMap) { - val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") - state[SAVE_INSTANCE_KEY] = sessionKeys - } -} diff --git a/changelog.d/880.bugfix b/changelog.d/880.bugfix new file mode 100644 index 0000000000..b6d46820a3 --- /dev/null +++ b/changelog.d/880.bugfix @@ -0,0 +1 @@ +Fix sliding sync loop restarts due to expirations. diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt new file mode 100644 index 0000000000..904cbeaed2 --- /dev/null +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt @@ -0,0 +1,87 @@ +/* + * Copyright (c) 2023 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.matrix.ui.di + +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap +import io.element.android.libraries.di.AppScope +import io.element.android.libraries.di.SingleIn +import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService +import io.element.android.libraries.matrix.api.core.SessionId +import kotlinx.coroutines.runBlocking +import timber.log.Timber +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject + +private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" + +@SingleIn(AppScope::class) +class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { + + private val sessionIdsToMatrixClient = ConcurrentHashMap() + + fun add(matrixClient: MatrixClient) { + sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient + } + + fun removeAll() { + sessionIdsToMatrixClient.clear() + } + + fun remove(sessionId: SessionId) { + sessionIdsToMatrixClient.remove(sessionId) + } + + fun isEmpty(): Boolean = sessionIdsToMatrixClient.isEmpty() + + fun knowSession(sessionId: SessionId): Boolean = sessionIdsToMatrixClient.containsKey(sessionId) + + fun getOrNull(sessionId: SessionId): MatrixClient? { + return sessionIdsToMatrixClient[sessionId] + } + + @Suppress("UNCHECKED_CAST") + fun restore(state: SavedStateMap?) { + Timber.d("Restore state") + if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { + Timber.w("Restore with non-empty map") + } + val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") + if (sessionIds.isNullOrEmpty()) return + // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. + runBlocking { + sessionIds.forEach { sessionId -> + Timber.d("Restore matrix session: $sessionId") + authenticationService.restoreSession(sessionId) + .onSuccess { matrixClient -> + add(matrixClient) + } + .onFailure { + Timber.e("Fail to restore session") + } + } + } + } + + fun save(state: MutableSavedStateMap) { + val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() + Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") + state[SAVE_INSTANCE_KEY] = sessionKeys + } +} diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt index cd0274016b..c001c4ab6e 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt @@ -16,6 +16,7 @@ package io.element.android.libraries.push.impl.notifications +import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder import io.element.android.libraries.androidutils.throttler.FirstThrottler import io.element.android.libraries.core.cache.CircularCache import io.element.android.libraries.core.coroutine.CoroutineDispatchers @@ -60,6 +61,7 @@ class DefaultNotificationDrawerManager @Inject constructor( private val dispatchers: CoroutineDispatchers, private val buildMeta: BuildMeta, private val matrixAuthenticationService: MatrixAuthenticationService, + private val matrixClientsHolder: MatrixClientsHolder, ) : NotificationDrawerManager { /** * Lazily initializes the NotificationState as we rely on having a current session in order to fetch the persisted queue of events. @@ -255,7 +257,7 @@ class DefaultNotificationDrawerManager @Inject constructor( val currentUser = tryOrNull( onError = { Timber.e(it, "Unable to retrieve info for user ${sessionId.value}") }, operation = { - val client = matrixAuthenticationService.restoreSession(sessionId).getOrNull() + val client = matrixClientsHolder.getOrNull(sessionId) // myUserDisplayName cannot be empty else NotificationCompat.MessagingStyle() will crash val myUserDisplayName = client?.loadUserDisplayName()?.getOrNull() ?: sessionId.value diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt index f0b70d8fac..74189276a1 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt @@ -16,6 +16,7 @@ package io.element.android.libraries.push.impl.notifications +import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.core.meta.BuildMeta import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService @@ -62,12 +63,13 @@ class NotifiableEventResolver @Inject constructor( private val matrixAuthenticationService: MatrixAuthenticationService, private val buildMeta: BuildMeta, private val clock: SystemClock, + private val matrixClientsHolder: MatrixClientsHolder, ) { suspend fun resolveEvent(sessionId: SessionId, roomId: RoomId, eventId: EventId): NotifiableEvent? { // Restore session - val session = matrixAuthenticationService.restoreSession(sessionId).getOrNull() ?: return null - val notificationService = session.notificationService() + val client = matrixClientsHolder.getOrNull(sessionId) ?: return null + val notificationService = client.notificationService() val notificationData = notificationService.getNotification( userId = sessionId, roomId = roomId, From e2549a8308cc478a645962f8a220a40281f4b5ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Mon, 17 Jul 2023 15:05:13 +0200 Subject: [PATCH 2/8] Try to centralise session restoration through `MatrixClientsHolder` --- .../io/element/android/appnav/RootFlowNode.kt | 10 ++--- .../impl/auth/AuthenticationException.kt | 2 +- .../matrix/ui/di/MatrixClientsHolder.kt | 37 ++++++++++++++----- .../DefaultNotificationDrawerManager.kt | 6 +-- .../notifications/NotifiableEventResolver.kt | 2 +- 5 files changed, 36 insertions(+), 21 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index 71f80dae3e..adc1c3384d 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -145,14 +145,10 @@ class RootFlowNode @AssistedInject constructor( onFailure: () -> Unit = {}, onSuccess: (SessionId) -> Unit = {}, ) { - // If the session is already known it'll be restored by the node hierarchy - if (matrixClientsHolder.knowSession(sessionId)) { - Timber.v("Session $sessionId already alive, no need to restore.") - return + runCatching { + matrixClientsHolder.requireSession(sessionId) } - authenticationService.restoreSession(sessionId) - .onSuccess { matrixClient -> - matrixClientsHolder.add(matrixClient) + .onSuccess { Timber.v("Succeed to restore session $sessionId") onSuccess(sessionId) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt index c264a95f67..26a9fba38f 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt @@ -35,6 +35,6 @@ fun Throwable.mapAuthenticationException(): Throwable { is RustAuthenticationException.OidcNotSupported -> AuthenticationException.OidcError("OidcNotSupported", message!!) */ - else -> this + else -> AuthenticationException.Generic(this.message ?: "Unknown error") } } diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt index 904cbeaed2..a6db7a1c9e 100644 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt +++ b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt @@ -21,12 +21,16 @@ import com.bumble.appyx.core.state.SavedStateMap import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.SingleIn import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.auth.AuthenticationException import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import timber.log.Timber import java.util.concurrent.ConcurrentHashMap import javax.inject.Inject +import kotlin.jvm.Throws private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" @@ -34,8 +38,9 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { private val sessionIdsToMatrixClient = ConcurrentHashMap() + private val restoreMutex = Mutex() - fun add(matrixClient: MatrixClient) { + private fun add(matrixClient: MatrixClient) { sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient } @@ -55,6 +60,27 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: return sessionIdsToMatrixClient[sessionId] } + @Throws(AuthenticationException::class) + suspend fun requireSession(sessionId: SessionId): MatrixClient { + return restoreMutex.withLock { + when (val matrixClient = sessionIdsToMatrixClient[sessionId]) { + null -> restore(sessionId).getOrThrow() + else -> matrixClient + } + } + } + + private suspend fun restore(sessionId: SessionId): Result { + Timber.d("Restore matrix session: $sessionId") + return authenticationService.restoreSession(sessionId) + .onSuccess { matrixClient -> + add(matrixClient) + } + .onFailure { + Timber.e("Fail to restore session") + } + } + @Suppress("UNCHECKED_CAST") fun restore(state: SavedStateMap?) { Timber.d("Restore state") @@ -67,14 +93,7 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. runBlocking { sessionIds.forEach { sessionId -> - Timber.d("Restore matrix session: $sessionId") - authenticationService.restoreSession(sessionId) - .onSuccess { matrixClient -> - add(matrixClient) - } - .onFailure { - Timber.e("Fail to restore session") - } + restore(sessionId) } } } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt index c001c4ab6e..dc484c29a3 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt @@ -257,11 +257,11 @@ class DefaultNotificationDrawerManager @Inject constructor( val currentUser = tryOrNull( onError = { Timber.e(it, "Unable to retrieve info for user ${sessionId.value}") }, operation = { - val client = matrixClientsHolder.getOrNull(sessionId) + val client = matrixClientsHolder.requireSession(sessionId) // myUserDisplayName cannot be empty else NotificationCompat.MessagingStyle() will crash - val myUserDisplayName = client?.loadUserDisplayName()?.getOrNull() ?: sessionId.value - val userAvatarUrl = client?.loadUserAvatarURLString()?.getOrNull() + val myUserDisplayName = client.loadUserDisplayName().getOrNull() ?: sessionId.value + val userAvatarUrl = client.loadUserAvatarURLString().getOrNull() MatrixUser( userId = sessionId, displayName = myUserDisplayName, diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt index 74189276a1..ad55bddb54 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt @@ -68,7 +68,7 @@ class NotifiableEventResolver @Inject constructor( suspend fun resolveEvent(sessionId: SessionId, roomId: RoomId, eventId: EventId): NotifiableEvent? { // Restore session - val client = matrixClientsHolder.getOrNull(sessionId) ?: return null + val client = matrixClientsHolder.requireSession(sessionId) val notificationService = client.notificationService() val notificationData = notificationService.getNotification( userId = sessionId, From abe7e952a3d8ae94f986151a239b39302086fbbd Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jul 2023 15:50:08 +0200 Subject: [PATCH 3/8] Map ClientException. --- .../matrix/api/exception/ClientException.kt | 22 +++++++++++++++ .../auth/RustMatrixAuthenticationService.kt | 3 ++- .../matrix/impl/exception/ClientException.kt | 27 +++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt create mode 100644 libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt new file mode 100644 index 0000000000..52dbd2eb12 --- /dev/null +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2023 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.matrix.api.exception + +sealed class ClientException(message: String) : Exception(message) { + class Generic(message: String) : ClientException(message) + class Other(message: String) : ClientException(message) +} diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt index bdb87b298c..5b8d175d40 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt @@ -29,6 +29,7 @@ import io.element.android.libraries.matrix.api.auth.MatrixHomeServerDetails import io.element.android.libraries.matrix.api.auth.OidcDetails import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.impl.RustMatrixClient +import io.element.android.libraries.matrix.impl.exception.mapClientException import io.element.android.libraries.network.useragent.UserAgentProvider import io.element.android.libraries.sessionstorage.api.SessionData import io.element.android.libraries.sessionstorage.api.SessionStore @@ -94,7 +95,7 @@ class RustMatrixAuthenticationService @Inject constructor( throw IllegalStateException("No session to restore with id $sessionId") } }.mapFailure { failure -> - failure.mapAuthenticationException() + failure.mapClientException() } } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt new file mode 100644 index 0000000000..a72755d129 --- /dev/null +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt @@ -0,0 +1,27 @@ +/* + * Copyright (c) 2023 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.matrix.impl.exception + +import io.element.android.libraries.matrix.api.exception.ClientException +import org.matrix.rustcomponents.sdk.ClientException as RustClientException + +fun Throwable.mapClientException(): Throwable { + return when (this) { + is RustClientException.Generic -> ClientException.Generic(msg) + else -> ClientException.Other(message ?: "Unknown error") + } +} From 2b679710d21c893f9ba3225e7a32b5cdaeb32ba0 Mon Sep 17 00:00:00 2001 From: ganfra Date: Mon, 17 Jul 2023 18:34:50 +0200 Subject: [PATCH 4/8] Rework a bit MatrixClientHolder and reintroduce cacheIndex... --- .../io/element/android/appnav/RootFlowNode.kt | 66 ++++------- .../android/appnav/di/MatrixClientsHolder.kt | 83 ++++++++++++++ .../android/appnav/root/RootNavState.kt | 22 ++++ .../appnav/root/RootNavStateFlowFactory.kt | 90 +++++++++++++++ .../impl/tasks/ClearCacheUseCase.kt | 1 - .../matrix/api/MatrixClientProvider.kt | 28 +++++ .../matrix/ui/di/MatrixClientsHolder.kt | 106 ------------------ .../DefaultNotificationDrawerManager.kt | 9 +- .../notifications/NotifiableEventResolver.kt | 22 ++-- 9 files changed, 255 insertions(+), 172 deletions(-) create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt create mode 100644 libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/MatrixClientProvider.kt delete mode 100644 libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt index adc1c3384d..089e956c61 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt @@ -37,15 +37,14 @@ import com.bumble.appyx.navmodel.backstack.operation.push import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode -import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder +import io.element.android.appnav.di.MatrixClientsHolder import io.element.android.appnav.intent.IntentResolver import io.element.android.appnav.intent.ResolvedIntent +import io.element.android.appnav.root.RootNavStateFlowFactory import io.element.android.appnav.root.RootPresenter import io.element.android.appnav.root.RootView -import io.element.android.features.login.api.LoginUserStory import io.element.android.features.login.api.oidc.OidcAction import io.element.android.features.login.api.oidc.OidcActionFlow -import io.element.android.features.preferences.api.CacheService import io.element.android.features.rageshake.api.bugreport.BugReportEntryPoint import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler @@ -57,29 +56,22 @@ import io.element.android.libraries.di.AppScope import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId import kotlinx.coroutines.flow.distinctUntilChanged - -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.launchIn -import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach -import kotlinx.coroutines.flow.onStart import kotlinx.parcelize.Parcelize import timber.log.Timber -import java.util.UUID @ContributesNode(AppScope::class) class RootFlowNode @AssistedInject constructor( @Assisted val buildContext: BuildContext, @Assisted plugins: List, private val authenticationService: MatrixAuthenticationService, - private val cacheService: CacheService, + private val navStateFlowFactory: RootNavStateFlowFactory, private val matrixClientsHolder: MatrixClientsHolder, private val presenter: RootPresenter, private val bugReportEntryPoint: BugReportEntryPoint, private val intentResolver: IntentResolver, private val oidcActionFlow: OidcActionFlow, - private val loginUserStory: LoginUserStory, ) : BackstackNode( backstack = BackStack( @@ -91,26 +83,25 @@ class RootFlowNode @AssistedInject constructor( ) { override fun onBuilt() { - matrixClientsHolder.restore(buildContext.savedStateMap) + matrixClientsHolder.restoreWithSavedState(buildContext.savedStateMap) super.onBuilt() - observeLoggedInState() + observeNavState() } override fun onSaveInstanceState(state: MutableSavedStateMap) { super.onSaveInstanceState(state) - matrixClientsHolder.save(state) + matrixClientsHolder.saveIntoSavedState(state) + navStateFlowFactory.saveIntoSavedState(state) } - private fun observeLoggedInState() { - combine( - cacheService.onClearedCacheEventFlow(), - isUserLoggedInFlow(), - ) { _, isLoggedIn -> isLoggedIn } - .onEach { isLoggedIn -> - Timber.v("isLoggedIn=$isLoggedIn") - if (isLoggedIn) { + private fun observeNavState() { + navStateFlowFactory.create(buildContext.savedStateMap) + .distinctUntilChanged() + .onEach { navState -> + Timber.v("navState=$navState") + if (navState.isLoggedIn) { tryToRestoreLatestSession( - onSuccess = { switchToLoggedInFlow(it) }, + onSuccess = { sessionId -> switchToLoggedInFlow(sessionId, navState.cacheIndex) }, onFailure = { switchToNotLoggedInFlow() } ) } else { @@ -120,19 +111,8 @@ class RootFlowNode @AssistedInject constructor( .launchIn(lifecycleScope) } - - private fun switchToLoggedInFlow(sessionId: SessionId) { - backstack.safeRoot(NavTarget.LoggedInFlow(sessionId)) - } - - private fun isUserLoggedInFlow(): Flow { - return combine( - authenticationService.isLoggedIn(), - loginUserStory.loginFlowIsDone - ) { isLoggedIn, loginFlowIsDone -> - isLoggedIn && loginFlowIsDone - } - .distinctUntilChanged() + private fun switchToLoggedInFlow(sessionId: SessionId, navId: Int) { + backstack.safeRoot(NavTarget.LoggedInFlow(sessionId, navId)) } private fun switchToNotLoggedInFlow() { @@ -145,9 +125,7 @@ class RootFlowNode @AssistedInject constructor( onFailure: () -> Unit = {}, onSuccess: (SessionId) -> Unit = {}, ) { - runCatching { - matrixClientsHolder.requireSession(sessionId) - } + matrixClientsHolder.getOrRestore(sessionId) .onSuccess { Timber.v("Succeed to restore session $sessionId") onSuccess(sessionId) @@ -200,7 +178,7 @@ class RootFlowNode @AssistedInject constructor( @Parcelize data class LoggedInFlow( val sessionId: SessionId, - val navId: UUID = UUID.randomUUID(), + val navId: Int ) : NavTarget @Parcelize @@ -274,11 +252,5 @@ class RootFlowNode @AssistedInject constructor( navTarget is NavTarget.LoggedInFlow && navTarget.sessionId == sessionId } } - - private fun CacheService.onClearedCacheEventFlow(): Flow { - return clearedCacheEventFlow - .onEach { sessionId -> matrixClientsHolder.remove(sessionId) } - .map { } - .onStart { emit((Unit)) } - } } + diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index 53a49aef9f..ac0eb60763 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -16,3 +16,86 @@ package io.element.android.appnav.di +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap +import com.squareup.anvil.annotations.ContributesBinding +import io.element.android.libraries.di.AppScope +import io.element.android.libraries.di.SingleIn +import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService +import io.element.android.libraries.matrix.api.core.SessionId +import io.element.android.libraries.matrix.api.MatrixClientProvider +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import timber.log.Timber +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject + +private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" + +@SingleIn(AppScope::class) +@ContributesBinding(AppScope::class) +class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService): MatrixClientProvider { + + private val sessionIdsToMatrixClient = ConcurrentHashMap() + private val restoreMutex = Mutex() + + fun removeAll() { + sessionIdsToMatrixClient.clear() + } + + fun remove(sessionId: SessionId) { + sessionIdsToMatrixClient.remove(sessionId) + } + + fun isEmpty(): Boolean = sessionIdsToMatrixClient.isEmpty() + + fun getOrNull(sessionId: SessionId): MatrixClient? { + return sessionIdsToMatrixClient[sessionId] + } + + override suspend fun getOrRestore(sessionId: SessionId): Result { + return restoreMutex.withLock { + when (val matrixClient = sessionIdsToMatrixClient[sessionId]) { + null -> restore(sessionId) + else -> Result.success(matrixClient) + } + } + } + + @Suppress("UNCHECKED_CAST") + fun restoreWithSavedState(state: SavedStateMap?) { + Timber.d("Restore state") + if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { + Timber.w("Restore with non-empty map") + } + val sessionIds = state[SAVE_INSTANCE_KEY] as? Array + Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") + if (sessionIds.isNullOrEmpty()) return + // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. + runBlocking { + sessionIds.forEach { sessionId -> + restore(sessionId) + } + } + } + + fun saveIntoSavedState(state: MutableSavedStateMap) { + val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() + Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") + state[SAVE_INSTANCE_KEY] = sessionKeys + } + + private suspend fun restore(sessionId: SessionId): Result { + Timber.d("Restore matrix session: $sessionId") + return authenticationService.restoreSession(sessionId) + .onSuccess { matrixClient -> + sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient + } + .onFailure { + Timber.e("Fail to restore session") + } + } + +} diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt new file mode 100644 index 0000000000..55edf96aee --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2023 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.appnav.root + +data class RootNavState( + val cacheIndex: Int, + val isLoggedIn: Boolean +) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt new file mode 100644 index 0000000000..84f50a835e --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2023 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.appnav.root + +import com.bumble.appyx.core.state.MutableSavedStateMap +import com.bumble.appyx.core.state.SavedStateMap +import io.element.android.appnav.di.MatrixClientsHolder +import io.element.android.features.login.api.LoginUserStory +import io.element.android.features.preferences.api.CacheService +import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.onEach +import javax.inject.Inject + +private const val SAVE_INSTANCE_KEY = "io.element.android.x.RootNavStateFlowFactory.SAVE_INSTANCE_KEY" + +class RootNavStateFlowFactory @Inject constructor( + private val authenticationService: MatrixAuthenticationService, + private val cacheService: CacheService, + private val matrixClientsHolder: MatrixClientsHolder, + private val loginUserStory: LoginUserStory, +) { + + private var currentCacheIndex = 0 + + fun create(savedStateMap: SavedStateMap?): Flow { + /** + * A flow of integer, where each time a clear cache is done, we have a new incremented value. + */ + val initialCacheIndex = savedStateMap.getCacheIndexOrDefault() + val cacheIndexFlow = cacheService.clearedCacheEventFlow + .onEach { sessionId -> + matrixClientsHolder.remove(sessionId) + } + .toIndexFlow(initialCacheIndex) + .onEach { cacheIndex -> + currentCacheIndex = cacheIndex + } + + return combine( + cacheIndexFlow, + isUserLoggedInFlow(), + ) { navId, isLoggedIn -> + RootNavState(navId, isLoggedIn) + } + } + + fun saveIntoSavedState(stateMap: MutableSavedStateMap) { + stateMap[SAVE_INSTANCE_KEY] = currentCacheIndex + } + + private fun isUserLoggedInFlow(): Flow { + return combine( + authenticationService.isLoggedIn(), + loginUserStory.loginFlowIsDone + ) { isLoggedIn, loginFlowIsDone -> + isLoggedIn && loginFlowIsDone + } + .distinctUntilChanged() + } + + private fun Flow.toIndexFlow(initialValue: Int): Flow = flow { + var index = initialValue + emit(initialValue) + collect { + emit(++index) + } + } + + private fun SavedStateMap?.getCacheIndexOrDefault(): Int { + return this?.get(SAVE_INSTANCE_KEY) as? Int ?: 0 + } +} diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt index 0ef0e02558..24c67a3824 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt @@ -27,7 +27,6 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.api.MatrixClient -import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.withContext import okhttp3.OkHttpClient import javax.inject.Inject diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/MatrixClientProvider.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/MatrixClientProvider.kt new file mode 100644 index 0000000000..44d1a1d1a6 --- /dev/null +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/MatrixClientProvider.kt @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2023 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.matrix.api + +import io.element.android.libraries.matrix.api.core.SessionId + +interface MatrixClientProvider { + /** + * Can be used to get or restore a MatrixClient with the given [SessionId]. + * If a [MatrixClient] is already in memory, it'll return it. Otherwise it'll try to restore one. + * Most of the time you want to use injected constructor instead of retrieving a MatrixClient with this provider. + */ + suspend fun getOrRestore(sessionId: SessionId): Result +} diff --git a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt b/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt deleted file mode 100644 index a6db7a1c9e..0000000000 --- a/libraries/matrixui/src/main/kotlin/io/element/android/libraries/matrix/ui/di/MatrixClientsHolder.kt +++ /dev/null @@ -1,106 +0,0 @@ -/* - * Copyright (c) 2023 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.matrix.ui.di - -import com.bumble.appyx.core.state.MutableSavedStateMap -import com.bumble.appyx.core.state.SavedStateMap -import io.element.android.libraries.di.AppScope -import io.element.android.libraries.di.SingleIn -import io.element.android.libraries.matrix.api.MatrixClient -import io.element.android.libraries.matrix.api.auth.AuthenticationException -import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService -import io.element.android.libraries.matrix.api.core.SessionId -import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.sync.Mutex -import kotlinx.coroutines.sync.withLock -import timber.log.Timber -import java.util.concurrent.ConcurrentHashMap -import javax.inject.Inject -import kotlin.jvm.Throws - -private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHolder.SaveInstanceKey" - -@SingleIn(AppScope::class) -class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) { - - private val sessionIdsToMatrixClient = ConcurrentHashMap() - private val restoreMutex = Mutex() - - private fun add(matrixClient: MatrixClient) { - sessionIdsToMatrixClient[matrixClient.sessionId] = matrixClient - } - - fun removeAll() { - sessionIdsToMatrixClient.clear() - } - - fun remove(sessionId: SessionId) { - sessionIdsToMatrixClient.remove(sessionId) - } - - fun isEmpty(): Boolean = sessionIdsToMatrixClient.isEmpty() - - fun knowSession(sessionId: SessionId): Boolean = sessionIdsToMatrixClient.containsKey(sessionId) - - fun getOrNull(sessionId: SessionId): MatrixClient? { - return sessionIdsToMatrixClient[sessionId] - } - - @Throws(AuthenticationException::class) - suspend fun requireSession(sessionId: SessionId): MatrixClient { - return restoreMutex.withLock { - when (val matrixClient = sessionIdsToMatrixClient[sessionId]) { - null -> restore(sessionId).getOrThrow() - else -> matrixClient - } - } - } - - private suspend fun restore(sessionId: SessionId): Result { - Timber.d("Restore matrix session: $sessionId") - return authenticationService.restoreSession(sessionId) - .onSuccess { matrixClient -> - add(matrixClient) - } - .onFailure { - Timber.e("Fail to restore session") - } - } - - @Suppress("UNCHECKED_CAST") - fun restore(state: SavedStateMap?) { - Timber.d("Restore state") - if (state == null || sessionIdsToMatrixClient.isNotEmpty()) return Unit.also { - Timber.w("Restore with non-empty map") - } - val sessionIds = state[SAVE_INSTANCE_KEY] as? Array - Timber.d("Restore matrix session keys = ${sessionIds?.map { it.value }}") - if (sessionIds.isNullOrEmpty()) return - // Not ideal but should only happens in case of process recreation. This ensure we restore all the active sessions before restoring the node graphs. - runBlocking { - sessionIds.forEach { sessionId -> - restore(sessionId) - } - } - } - - fun save(state: MutableSavedStateMap) { - val sessionKeys = sessionIdsToMatrixClient.keys.toTypedArray() - Timber.d("Save matrix session keys = ${sessionKeys.map { it.value }}") - state[SAVE_INSTANCE_KEY] = sessionKeys - } -} diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt index dc484c29a3..8851b8eea8 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt @@ -16,7 +16,6 @@ package io.element.android.libraries.push.impl.notifications -import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder import io.element.android.libraries.androidutils.throttler.FirstThrottler import io.element.android.libraries.core.cache.CircularCache import io.element.android.libraries.core.coroutine.CoroutineDispatchers @@ -24,12 +23,12 @@ import io.element.android.libraries.core.data.tryOrNull import io.element.android.libraries.core.meta.BuildMeta import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.SingleIn -import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.api.core.ThreadId import io.element.android.libraries.matrix.api.user.MatrixUser +import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.push.api.notifications.NotificationDrawerManager import io.element.android.libraries.push.api.store.PushDataStore import io.element.android.libraries.push.impl.notifications.model.NotifiableEvent @@ -60,8 +59,7 @@ class DefaultNotificationDrawerManager @Inject constructor( private val coroutineScope: CoroutineScope, private val dispatchers: CoroutineDispatchers, private val buildMeta: BuildMeta, - private val matrixAuthenticationService: MatrixAuthenticationService, - private val matrixClientsHolder: MatrixClientsHolder, + private val matrixClientProvider: MatrixClientProvider, ) : NotificationDrawerManager { /** * Lazily initializes the NotificationState as we rely on having a current session in order to fetch the persisted queue of events. @@ -257,8 +255,7 @@ class DefaultNotificationDrawerManager @Inject constructor( val currentUser = tryOrNull( onError = { Timber.e(it, "Unable to retrieve info for user ${sessionId.value}") }, operation = { - val client = matrixClientsHolder.requireSession(sessionId) - + val client = matrixClientProvider.getOrRestore(sessionId).getOrThrow() // myUserDisplayName cannot be empty else NotificationCompat.MessagingStyle() will crash val myUserDisplayName = client.loadUserDisplayName().getOrNull() ?: sessionId.value val userAvatarUrl = client.loadUserAvatarURLString().getOrNull() diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt index ad55bddb54..dbdddf1ac9 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventResolver.kt @@ -16,10 +16,8 @@ package io.element.android.libraries.push.impl.notifications -import io.element.android.libraries.matrix.ui.di.MatrixClientsHolder import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.core.meta.BuildMeta -import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.SessionId @@ -36,6 +34,7 @@ import io.element.android.libraries.matrix.api.timeline.item.event.NoticeMessage import io.element.android.libraries.matrix.api.timeline.item.event.TextMessageType import io.element.android.libraries.matrix.api.timeline.item.event.UnknownMessageType import io.element.android.libraries.matrix.api.timeline.item.event.VideoMessageType +import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.push.impl.R import io.element.android.libraries.push.impl.log.pushLoggerTag import io.element.android.libraries.push.impl.notifications.model.FallbackNotifiableEvent @@ -60,26 +59,25 @@ class NotifiableEventResolver @Inject constructor( private val stringProvider: StringProvider, // private val noticeEventFormatter: NoticeEventFormatter, // private val displayableEventFormatter: DisplayableEventFormatter, - private val matrixAuthenticationService: MatrixAuthenticationService, private val buildMeta: BuildMeta, private val clock: SystemClock, - private val matrixClientsHolder: MatrixClientsHolder, + private val matrixClientProvider: MatrixClientProvider, ) { suspend fun resolveEvent(sessionId: SessionId, roomId: RoomId, eventId: EventId): NotifiableEvent? { // Restore session - val client = matrixClientsHolder.requireSession(sessionId) + val client = matrixClientProvider.getOrRestore(sessionId).getOrNull() ?: return null val notificationService = client.notificationService() val notificationData = notificationService.getNotification( - userId = sessionId, - roomId = roomId, - eventId = eventId, + userId = sessionId, + roomId = roomId, + eventId = eventId, // FIXME should be true in the future, but right now it's broken // (https://github.com/vector-im/element-x-android/issues/640#issuecomment-1612913658) - filterByPushRules = false, - ).onFailure { - Timber.tag(loggerTag.value).e(it, "Unable to resolve event: $eventId.") - }.getOrNull() + filterByPushRules = false, + ).onFailure { + Timber.tag(loggerTag.value).e(it, "Unable to resolve event: $eventId.") + }.getOrNull() // TODO this notificationData is not always valid at the moment, sometimes the Rust SDK can't fetch the matching event return notificationData?.asNotifiableEvent(sessionId) From 07ab919367849a0fccaacf3bb33e8c9321cd59e3 Mon Sep 17 00:00:00 2001 From: ganfra Date: Mon, 17 Jul 2023 21:32:07 +0200 Subject: [PATCH 5/8] MatrixClientHolders: some more cleanup --- .../android/appnav/di/MatrixClientsHolder.kt | 9 ++--- .../android/appnav/root/RootNavState.kt | 10 +++++ .../appnav/root/RootNavStateFlowFactory.kt | 39 ++++++++++++------- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt index ac0eb60763..3e36e7d692 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/di/MatrixClientsHolder.kt @@ -22,9 +22,9 @@ import com.squareup.anvil.annotations.ContributesBinding import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.SingleIn import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService import io.element.android.libraries.matrix.api.core.SessionId -import io.element.android.libraries.matrix.api.MatrixClientProvider import kotlinx.coroutines.runBlocking import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -36,7 +36,7 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.di.MatrixClientsHold @SingleIn(AppScope::class) @ContributesBinding(AppScope::class) -class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService): MatrixClientProvider { +class MatrixClientsHolder @Inject constructor(private val authenticationService: MatrixAuthenticationService) : MatrixClientProvider { private val sessionIdsToMatrixClient = ConcurrentHashMap() private val restoreMutex = Mutex() @@ -49,15 +49,13 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: sessionIdsToMatrixClient.remove(sessionId) } - fun isEmpty(): Boolean = sessionIdsToMatrixClient.isEmpty() - fun getOrNull(sessionId: SessionId): MatrixClient? { return sessionIdsToMatrixClient[sessionId] } override suspend fun getOrRestore(sessionId: SessionId): Result { return restoreMutex.withLock { - when (val matrixClient = sessionIdsToMatrixClient[sessionId]) { + when (val matrixClient = getOrNull(sessionId)) { null -> restore(sessionId) else -> Result.success(matrixClient) } @@ -97,5 +95,4 @@ class MatrixClientsHolder @Inject constructor(private val authenticationService: Timber.e("Fail to restore session") } } - } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt index 55edf96aee..ed3ac15972 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavState.kt @@ -16,7 +16,17 @@ package io.element.android.appnav.root +/** + * [RootNavState] produced by [RootNavStateFlowFactory]. + */ data class RootNavState( + /** + * This value is incremented when a clear cache is done. + * Can be useful to track to force ui state to re-render + */ val cacheIndex: Int, + /** + * true if we are currently loggedIn. + */ val isLoggedIn: Boolean ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt index 84f50a835e..0e8d93b0c9 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/root/RootNavStateFlowFactory.kt @@ -31,6 +31,10 @@ import javax.inject.Inject private const val SAVE_INSTANCE_KEY = "io.element.android.x.RootNavStateFlowFactory.SAVE_INSTANCE_KEY" +/** + * This class is responsible for creating a flow of [RootNavState]. + * It gathers data from multiple datasource and creates a unique one. + */ class RootNavStateFlowFactory @Inject constructor( private val authenticationService: MatrixAuthenticationService, private val cacheService: CacheService, @@ -41,11 +45,24 @@ class RootNavStateFlowFactory @Inject constructor( private var currentCacheIndex = 0 fun create(savedStateMap: SavedStateMap?): Flow { - /** - * A flow of integer, where each time a clear cache is done, we have a new incremented value. - */ + return combine( + cacheIndexFlow(savedStateMap), + isUserLoggedInFlow(), + ) { cacheIndex, isLoggedIn -> + RootNavState(cacheIndex = cacheIndex, isLoggedIn = isLoggedIn) + } + } + + fun saveIntoSavedState(stateMap: MutableSavedStateMap) { + stateMap[SAVE_INSTANCE_KEY] = currentCacheIndex + } + + /** + * @return a flow of integer, where each time a clear cache is done, we have a new incremented value. + */ + private fun cacheIndexFlow(savedStateMap: SavedStateMap?): Flow { val initialCacheIndex = savedStateMap.getCacheIndexOrDefault() - val cacheIndexFlow = cacheService.clearedCacheEventFlow + return cacheService.clearedCacheEventFlow .onEach { sessionId -> matrixClientsHolder.remove(sessionId) } @@ -53,17 +70,6 @@ class RootNavStateFlowFactory @Inject constructor( .onEach { cacheIndex -> currentCacheIndex = cacheIndex } - - return combine( - cacheIndexFlow, - isUserLoggedInFlow(), - ) { navId, isLoggedIn -> - RootNavState(navId, isLoggedIn) - } - } - - fun saveIntoSavedState(stateMap: MutableSavedStateMap) { - stateMap[SAVE_INSTANCE_KEY] = currentCacheIndex } private fun isUserLoggedInFlow(): Flow { @@ -76,6 +82,9 @@ class RootNavStateFlowFactory @Inject constructor( .distinctUntilChanged() } + /** + * @return a flow of integer that increments the value by one each time a new element is emitted upstream. + */ private fun Flow.toIndexFlow(initialValue: Int): Flow = flow { var index = initialValue emit(initialValue) From 48277d095ab432d5d828bdfeaa0d0ece7b7c28a2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jul 2023 22:03:21 +0200 Subject: [PATCH 6/8] Change return type (mostly for clarity) --- .../libraries/matrix/impl/auth/AuthenticationException.kt | 2 +- .../android/libraries/matrix/impl/exception/ClientException.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt index 26a9fba38f..f0feb2857d 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/AuthenticationException.kt @@ -19,7 +19,7 @@ package io.element.android.libraries.matrix.impl.auth import io.element.android.libraries.matrix.api.auth.AuthenticationException import org.matrix.rustcomponents.sdk.AuthenticationException as RustAuthenticationException -fun Throwable.mapAuthenticationException(): Throwable { +fun Throwable.mapAuthenticationException(): AuthenticationException { return when (this) { is RustAuthenticationException.ClientMissing -> AuthenticationException.ClientMissing(this.message!!) is RustAuthenticationException.Generic -> AuthenticationException.Generic(this.message!!) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt index a72755d129..6efca88f9b 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/exception/ClientException.kt @@ -19,7 +19,7 @@ package io.element.android.libraries.matrix.impl.exception import io.element.android.libraries.matrix.api.exception.ClientException import org.matrix.rustcomponents.sdk.ClientException as RustClientException -fun Throwable.mapClientException(): Throwable { +fun Throwable.mapClientException(): ClientException { return when (this) { is RustClientException.Generic -> ClientException.Generic(msg) else -> ClientException.Other(message ?: "Unknown error") From af17a5646ce6f61adbf06f036ebbd00efb2e57a4 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jul 2023 22:17:04 +0200 Subject: [PATCH 7/8] Ignore RootNavState regarding koverage. --- build.gradle.kts | 1 + 1 file changed, 1 insertion(+) diff --git a/build.gradle.kts b/build.gradle.kts index f8eda34bd8..3348a47c55 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -247,6 +247,7 @@ koverMerged { target = kotlinx.kover.api.VerificationTarget.CLASS overrideClassFilter { includes += "*State" + excludes += "io.element.android.appnav.root.RootNavState*" excludes += "io.element.android.libraries.matrix.api.timeline.item.event.OtherState$*" excludes += "io.element.android.libraries.matrix.api.timeline.item.event.EventSendState$*" excludes += "io.element.android.libraries.matrix.api.room.RoomMembershipState*" From 207a20b67c4e5ea7f932391bfd9fcbf2265241c3 Mon Sep 17 00:00:00 2001 From: ganfra Date: Mon, 17 Jul 2023 23:27:18 +0200 Subject: [PATCH 8/8] RoomFlowNode: use newRoot instead of safeRoot as in this case it can create a race condition where we end up not switching node --- .../kotlin/io/element/android/appnav/room/RoomFlowNode.kt | 6 +++--- .../android/libraries/matrix/impl/RustMatrixClient.kt | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/room/RoomFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/room/RoomFlowNode.kt index a03b6d61a2..20ec9f48b4 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/room/RoomFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/room/RoomFlowNode.kt @@ -32,11 +32,11 @@ import com.bumble.appyx.core.node.node import com.bumble.appyx.core.plugin.Plugin import com.bumble.appyx.core.plugin.plugins import com.bumble.appyx.navmodel.backstack.BackStack +import com.bumble.appyx.navmodel.backstack.operation.newRoot import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode import io.element.android.appnav.NodeLifecycleCallback -import io.element.android.appnav.safeRoot import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.libraries.architecture.BackstackNode @@ -92,9 +92,9 @@ class RoomFlowNode @AssistedInject constructor( .distinctUntilChanged() .onEach { isLoaded -> if (isLoaded) { - backstack.safeRoot(NavTarget.Loaded) + backstack.newRoot(NavTarget.Loaded) } else { - backstack.safeRoot(NavTarget.Loading) + backstack.newRoot(NavTarget.Loading) } }.launchIn(lifecycleScope) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt index d7195ebf11..a467a161e0 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt @@ -165,8 +165,10 @@ class RustMatrixClient constructor( val cachedRoomListItem = roomListService.roomOrNull(roomId.value) val fullRoom = cachedRoomListItem?.fullRoom() if (cachedRoomListItem == null || fullRoom == null) { + Timber.d("No room cached for $roomId") null } else { + Timber.d("Found room cached for $roomId") Pair(cachedRoomListItem, fullRoom) } }