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 <[email protected]>
element-hq · Sep 18, 2024 · c08b8c0 · c08b8c0
1 parent dbc4c8f
commit c08b8c0
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 20 deletions.
diff --git a/...mpl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustClientSessionDelegate.kt b/...mpl/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/.../matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/.../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<MatrixUser> = 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<Unit> = 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/...rix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt b/...rix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/sync/RustSyncService.kt
@@ -16,28 +16,47 @@ 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 {
         Timber.d("Start sync failed: $it")
     }
 
     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<SyncState> =
         innerSyncService.stateFlow()
             .map(SyncServiceState::toSyncState)