Catch unhandled exception in video client scope (#1137)

* Treat EOFException errors as temporary * Spotless * Add the concept of `safeCall` to ensure uncaught exceptions don't crash the SDK. * Add the concept of `safeCall` to ensure uncaught exceptions don't crash the SDK. * Correctly connect / disconnect based on active call state * When the socket is connected don't reconnect * Spotless and API * Add extra logging * Add exception handling for scope in service and in the video client to ensure any uncaught exceptions are not crashing * Spotless and API dump * Add CoroutineName to exception handler * Remove CrashDebug logging * Spotless * When an error is 401 during a connection phase, don't treat it as a `ConnectionError` which will allow for refresh token --------- Co-authored-by: Liviu Timar <[email protected]>
GetStream · Jul 11, 2024 · a533306 · a533306
1 parent 98d467f
commit a533306
Show file tree

Hide file tree

Showing 6 changed files with 251 additions and 68 deletions.
diff --git a/stream-video-android-core/api/stream-video-android-core.api b/stream-video-android-core/api/stream-video-android-core.api
@@ -266,18 +266,16 @@ public final class io/getstream/video/android/core/ClientState {
 	public final fun addRingingCall (Lio/getstream/video/android/core/Call;Lio/getstream/video/android/core/RingingState;)V
 	public final fun getActiveCall ()Lkotlinx/coroutines/flow/StateFlow;
 	public final fun getConnection ()Lkotlinx/coroutines/flow/StateFlow;
+	public final fun getLogger ()Lio/getstream/log/TaggedLogger;
 	public final fun getRingingCall ()Lkotlinx/coroutines/flow/StateFlow;
 	public final fun getUser ()Lkotlinx/coroutines/flow/StateFlow;
 	public final fun handleEvent (Lorg/openapitools/client/models/VideoEvent;)V
+	public final fun hasActiveOrRingingCall ()Z
 	public final fun removeActiveCall ()V
 	public final fun removeRingingCall ()V
 	public final fun setActiveCall (Lio/getstream/video/android/core/Call;)V
 }
 
-public final class io/getstream/video/android/core/ClientStateKt {
-	public static final fun formatAsTitle (Lio/getstream/video/android/core/ConnectionState;Landroid/content/Context;)Ljava/lang/String;
-}
-
 public abstract interface class io/getstream/video/android/core/ConnectionState {
 }
 
@@ -4460,6 +4458,7 @@ public final class io/getstream/video/android/core/sounds/Sounds {
 }
 
 public final class io/getstream/video/android/core/utils/AndroidUtilsKt {
+	public static final fun safeCall (Ljava/lang/Object;Lkotlin/jvm/functions/Function0;)Ljava/lang/Object;
 	public static final fun shouldShowRequestPermissionsRationale (Landroid/app/Activity;[Ljava/lang/String;)Z
 }
 

diff --git a/stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt b/stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/ClientState.kt
@@ -16,10 +16,11 @@
 
 package io.getstream.video.android.core
 
-import android.content.Context
 import androidx.compose.runtime.Stable
 import androidx.core.content.ContextCompat
+import io.getstream.log.taggedLogger
 import io.getstream.video.android.core.notifications.internal.service.CallService
+import io.getstream.video.android.core.utils.safeCall
 import io.getstream.video.android.model.StreamCallId
 import io.getstream.video.android.model.User
 import kotlinx.coroutines.flow.MutableStateFlow
@@ -52,6 +53,9 @@ public sealed interface RingingState {
 
 @Stable
 class ClientState(client: StreamVideo) {
+
+    val logger by taggedLogger("ClientState")
+
     /**
      * Current user object
      */
@@ -80,11 +84,21 @@ class ClientState(client: StreamVideo) {
 
     internal val clientImpl = client as StreamVideoImpl
 
+    /**
+     * Returns true if there is an active or ringing call
+     */
+    fun hasActiveOrRingingCall(): Boolean = safeCall(false) {
+        val hasActiveCall = _activeCall.value != null
+        val hasRingingCall = _ringingCall.value != null
+        val activeOrRingingCall = hasActiveCall || hasRingingCall
+        logger.d { "[hasActiveOrRingingCall] active: $hasActiveCall, ringing: $hasRingingCall" }
+        activeOrRingingCall
+    }
+
     /**
      * Handles the events for the client state.
      * Most event logic happens in the Call instead of the client
      */
-
     fun handleEvent(event: VideoEvent) {
         // mark connected
         if (event is ConnectedEvent) {
@@ -162,12 +176,3 @@ class ClientState(client: StreamVideo) {
         }
     }
 }
-
-public fun ConnectionState.formatAsTitle(context: Context): String = when (this) {
-    ConnectionState.PreConnect -> "Connecting.."
-    ConnectionState.Loading -> "Loading.."
-    ConnectionState.Connected -> "Connected"
-    ConnectionState.Reconnecting -> "Reconnecting.."
-    ConnectionState.Disconnected -> "Disconnected"
-    is ConnectionState.Failed -> "Failed"
-}
diff --git a/stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoImpl.kt b/stream-video-android-core/src/main/kotlin/io/getstream/video/android/core/StreamVideoImpl.kt
@@ -54,12 +54,16 @@ import io.getstream.video.android.core.sounds.Sounds
 import io.getstream.video.android.core.utils.DebugInfo
 import io.getstream.video.android.core.utils.LatencyResult
 import io.getstream.video.android.core.utils.getLatencyMeasurementsOKHttp
+import io.getstream.video.android.core.utils.safeCall
+import io.getstream.video.android.core.utils.safeSuspendingCall
 import io.getstream.video.android.core.utils.toEdge
 import io.getstream.video.android.core.utils.toQueriedCalls
 import io.getstream.video.android.core.utils.toQueriedMembers
 import io.getstream.video.android.model.ApiKey
 import io.getstream.video.android.model.Device
 import io.getstream.video.android.model.User
+import kotlinx.coroutines.CoroutineExceptionHandler
+import kotlinx.coroutines.CoroutineName
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Deferred
 import kotlinx.coroutines.Dispatchers
@@ -156,7 +160,15 @@ internal class StreamVideoImpl internal constructor(
     /** the state for the client, includes the current user */
     override val state = ClientState(this)
 
-    internal val scope = CoroutineScope(_scope.coroutineContext + SupervisorJob())
+    private val coroutineExceptionHandler =
+        CoroutineExceptionHandler { coroutineContext, exception ->
+            val coroutineName = coroutineContext[CoroutineName]?.name ?: "unknown"
+            logger.e(exception) {
+                "[StreamVideo#Scope] Uncaught exception in coroutine $coroutineName: $exception"
+            }
+        }
+    internal val scope =
+        CoroutineScope(_scope.coroutineContext + SupervisorJob() + coroutineExceptionHandler)
 
     /** if true we fail fast on errors instead of logging them */
     var developmentMode = true
@@ -310,8 +322,8 @@ internal class StreamVideoImpl internal constructor(
         return sub
     }
 
-    override suspend fun connectIfNotAlreadyConnected() {
-        if (connectionModule.coordinatorSocket.connectionState.value != SocketState.NotConnected && connectionModule.coordinatorSocket.connectionState.value != SocketState.Connecting) {
+    override suspend fun connectIfNotAlreadyConnected() = safeSuspendingCall {
+        if (connectionModule.coordinatorSocket.canConnect()) {
             connectionModule.coordinatorSocket.connect()
         }
     }
@@ -323,39 +335,44 @@ internal class StreamVideoImpl internal constructor(
         lifecycle,
         object : LifecycleHandler {
             override fun started() {
-                scope.launch {
-                    // We should only connect if we were previously connected
-                    if (connectionModule.coordinatorSocket.connectionState.value != SocketState.NotConnected) {
-                        connectionModule.coordinatorSocket.connect()
-                    }
+                scope.launch(CoroutineName("lifecycleObserver.started")) {
+                    connectIfNotAlreadyConnected()
                 }
             }
 
             override fun stopped() {
-                // We should only disconnect if we were previously connected
-                // Also don't disconnect the socket if we are in an active call
-                if (connectionModule.coordinatorSocket.connectionState.value != SocketState.NotConnected && state.activeCall.value == null) {
-                    connectionModule.coordinatorSocket.disconnect(
-                        PersistentSocket.DisconnectReason.ByRequest,
-                    )
+                safeCall {
+                    // We should only disconnect if we were previously connected
+                    // Also don't disconnect the socket if we are in an active call
+                    val socketDecision = connectionModule.coordinatorSocket.canDisconnect()
+                    val activeCallDecision = state.hasActiveOrRingingCall()
+                    val decision = socketDecision && !activeCallDecision
+                    logger.d {
+                        "[lifecycle#stopped] Decision to disconnect: $decision, caused by socket state: $socketDecision, active or ringing call: $activeCallDecision"
+                    }
+                    if (decision) {
+                        connectionModule.coordinatorSocket.disconnect(
+                            PersistentSocket.DisconnectReason.ByRequest,
+                        )
+                    }
                 }
             }
         },
     )
 
     init {
 
-        scope.launch(Dispatchers.Main.immediate) {
+        scope.launch(Dispatchers.Main.immediate + CoroutineName("init#lifecycleObserver.observe")) {
             lifecycleObserver.observe()
         }
 
         // listen to socket events and errors
-        scope.launch {
+        scope.launch(CoroutineName("init#coordinatorSocket.events.collect")) {
             connectionModule.coordinatorSocket.events.collect {
                 fireEvent(it)
             }
         }
-        scope.launch {
+        scope.launch(CoroutineName("init#coordinatorSocket.errors.collect")) {
             connectionModule.coordinatorSocket.errors.collect { throwable ->
                 if (throwable is ConnectException) {
                     state.handleError(throwable)
@@ -367,7 +384,7 @@ internal class StreamVideoImpl internal constructor(
             }
         }
 
-        scope.launch {
+        scope.launch(CoroutineName("init#coordinatorSocket.connectionState.collect")) {
             connectionModule.coordinatorSocket.connectionState.collect { it ->
                 // If the socket is reconnected then we have a new connection ID.
                 // We need to re-watch every watched call with the new connection ID
@@ -1009,7 +1026,9 @@ internal class StreamVideoImpl internal constructor(
      * @see StreamVideo.logOut
      */
     override fun logOut() {
-        scope.launch { streamNotificationManager.deviceTokenStorage.clear() }
+        scope.launch(
+            CoroutineName("logOut"),
+        ) { streamNotificationManager.deviceTokenStorage.clear() }
     }
 
     override fun call(type: String, id: String): Call {

diff --git a/...main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt b/...main/kotlin/io/getstream/video/android/core/notifications/internal/service/CallService.kt
@@ -42,6 +42,7 @@ import io.getstream.video.android.core.notifications.internal.receivers.ToggleCa
 import io.getstream.video.android.model.StreamCallId
 import io.getstream.video.android.model.streamCallDisplayName
 import io.getstream.video.android.model.streamCallId
+import kotlinx.coroutines.CoroutineExceptionHandler
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.cancel
@@ -60,7 +61,10 @@ internal class CallService : Service() {
     private var callId: StreamCallId? = null
 
     // Service scope
-    private val serviceScope: CoroutineScope = CoroutineScope(Dispatchers.IO)
+    val handler = CoroutineExceptionHandler { _, exception ->
+        logger.e(exception) { "[CallService#Scope] Uncaught exception: $exception" }
+    }
+    private val serviceScope: CoroutineScope = CoroutineScope(Dispatchers.IO + handler)
 
     // Camera handling receiver
     private val toggleCameraBroadcastReceiver = ToggleCameraBroadcastReceiver(serviceScope)