From 8c2a6a54af143c0bd8a35d2f538a054bac4d76af Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Wed, 18 Sep 2024 13:54:54 +0200 Subject: [PATCH] Make sure the logout action doesn't cause a crash (#3480) * Make sure the logout doesn't cause a crash Some reasons why this could happen: 1. The `ClientDelegate` could receive a `didReceiveAuthError` callback call on a logout, which could trigger another logout when every Rust object had already been destroyed. 2. Even though we stop the sync before logging out, `LoggedInFlowNode` will try to start it again automatically when it detects we still have internet connection. Making sure to unregister the delegate should fix the first part of the issue. For the other one, adding `RustSyncService.isServiceReady` to check if we should start/stop the service, which is enabled by default and set to false on destroy should help. * Apply the same patch on account deactivation. --------- Co-authored-by: Benoit Marty --- .../matrix/impl/RustClientSessionDelegate.kt | 1 - .../libraries/matrix/impl/RustMatrixClient.kt | 44 ++++++++++++------- .../matrix/impl/sync/RustSyncService.kt | 23 +++++++++- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustClientSessionDelegate.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustClientSessionDelegate.kt index 03a59203e5..dcdb719511 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustClientSessionDelegate.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustClientSessionDelegate.kt @@ -50,7 +50,6 @@ class RustClientSessionDelegate( */ fun bindClient(client: RustMatrixClient) { this.client = client - client.setDelegate(this) } override fun saveSessionInKeychain(session: Session) { 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 a5c5f1f442..a49db1ccd2 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 @@ -122,7 +122,7 @@ class RustMatrixClient( private val baseDirectory: File, baseCacheDirectory: File, private val clock: SystemClock, - sessionDelegate: RustClientSessionDelegate, + private val sessionDelegate: RustClientSessionDelegate, ) : MatrixClient { override val sessionId: UserId = UserId(client.userId()) override val deviceId: DeviceId = DeviceId(client.deviceId()) @@ -195,7 +195,7 @@ class RustMatrixClient( private val roomMembershipObserver = RoomMembershipObserver() - private val clientDelegateTaskHandle: TaskHandle? = client.setDelegate(sessionDelegate) + private var clientDelegateTaskHandle: TaskHandle? = client.setDelegate(sessionDelegate) private val _userProfile: MutableStateFlow = MutableStateFlow( MatrixUser( @@ -449,12 +449,12 @@ class RustMatrixClient( override fun close() { appCoroutineScope.launch { roomFactory.destroy() + rustSyncService.destroy() } sessionCoroutineScope.cancel() clientDelegateTaskHandle?.cancelAndDestroy() notificationSettingsService.destroy() verificationService.destroy() - syncService.destroy() innerRoomListService.destroy() notificationClient.destroy() notificationProcessSetup.destroy() @@ -473,7 +473,9 @@ class RustMatrixClient( override suspend fun logout(userInitiated: Boolean, ignoreSdkError: Boolean): String? { var result: String? = null - syncService.stop() + // Remove current delegate so we don't receive an auth error + clientDelegateTaskHandle?.cancelAndDestroy() + clientDelegateTaskHandle = null withContext(sessionDispatcher) { if (userInitiated) { try { @@ -482,12 +484,15 @@ class RustMatrixClient( if (ignoreSdkError) { Timber.e(failure, "Fail to call logout on HS. Still delete local files.") } else { + // If the logout failed we need to restore the delegate + clientDelegateTaskHandle = client.setDelegate(sessionDelegate) Timber.e(failure, "Fail to call logout on HS.") throw failure } } } close() + deleteSessionDirectory(deleteCryptoDb = true) if (userInitiated) { sessionStore.removeSession(sessionId.value) @@ -506,7 +511,9 @@ class RustMatrixClient( override suspend fun deactivateAccount(password: String, eraseData: Boolean): Result = withContext(sessionDispatcher) { Timber.w("Deactivating account") - syncService.stop() + // Remove current delegate so we don't receive an auth error + clientDelegateTaskHandle?.cancelAndDestroy() + clientDelegateTaskHandle = null runCatching { // First call without AuthData, should fail val firstAttempt = runCatching { @@ -518,15 +525,22 @@ class RustMatrixClient( if (firstAttempt.isFailure) { Timber.w(firstAttempt.exceptionOrNull(), "Expected failure, try again") // This is expected, try again with the password - client.deactivateAccount( - authData = AuthData.Password( - passwordDetails = AuthDataPasswordDetails( - identifier = sessionId.value, - password = password, + runCatching { + client.deactivateAccount( + authData = AuthData.Password( + passwordDetails = AuthDataPasswordDetails( + identifier = sessionId.value, + password = password, + ), ), - ), - eraseData = eraseData, - ) + eraseData = eraseData, + ) + }.onFailure { + Timber.e(it, "Failed to deactivate account") + // If the deactivation failed we need to restore the delegate + clientDelegateTaskHandle = client.setDelegate(sessionDelegate) + throw it + } } close() deleteSessionDirectory(deleteCryptoDb = true) @@ -592,10 +606,6 @@ class RustMatrixClient( return client.session().slidingSyncVersion == SlidingSyncVersion.Native } - internal fun setDelegate(delegate: RustClientSessionDelegate) { - client.setDelegate(delegate) - } - private suspend fun File.getCacheSize( includeCryptoDb: Boolean = false, ): Long = withContext(sessionDispatcher) { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt index cf52609fa0..0a859ee146 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt @@ -16,15 +16,22 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.stateIn -import org.matrix.rustcomponents.sdk.SyncServiceInterface import org.matrix.rustcomponents.sdk.SyncServiceState import timber.log.Timber +import java.util.concurrent.atomic.AtomicBoolean +import org.matrix.rustcomponents.sdk.SyncService as InnerSyncService class RustSyncService( - private val innerSyncService: SyncServiceInterface, + private val innerSyncService: InnerSyncService, sessionCoroutineScope: CoroutineScope ) : SyncService { + private val isServiceReady = AtomicBoolean(true) + override suspend fun startSync() = runCatching { + if (!isServiceReady.get()) { + Timber.d("Can't start sync: service is not ready") + return@runCatching + } Timber.i("Start sync") innerSyncService.start() }.onFailure { @@ -32,12 +39,24 @@ class RustSyncService( } override suspend fun stopSync() = runCatching { + if (!isServiceReady.get()) { + Timber.d("Can't stop sync: service is not ready") + return@runCatching + } Timber.i("Stop sync") innerSyncService.stop() }.onFailure { Timber.d("Stop sync failed: $it") } + suspend fun destroy() { + // If the service was still running, stop it + stopSync() + Timber.d("Destroying sync service") + isServiceReady.set(false) + innerSyncService.destroy() + } + override val syncState: StateFlow = innerSyncService.stateFlow() .map(SyncServiceState::toSyncState)