Refactor sendData to return Result instead of throwing exceptions (#703)

* Switch away from throwing exceptions internally where not needed to avoid crashes.

* changeset

Author: davidliu

.changeset/soft-pugs-fetch.md

@@ -0,0 +1,6 @@
+---
+"client-sdk-android": minor
+---
+
+Refactor some internal data message sending methods to use Result instead of throwing Exceptions to
+fix crashes.

livekit-android-sdk/src/main/java/io/livekit/android/audio/PreconnectAudioBuffer.kt

@@ -148,7 +148,10 @@ internal constructor(timeout: Duration) : AudioTrackSink {
         )
 
         try {
-            sender.write(audioData)
+            val result = sender.write(audioData)
+            if (result.isFailure) {
+                result.exceptionOrNull()?.let { throw it }
+            }
             sender.close()
         } catch (e: Exception) {
             sender.close(e.localizedMessage)

livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt

@@ -17,6 +17,7 @@
 package io.livekit.android.room
 
 import android.os.SystemClock
+import androidx.annotation.CheckResult
 import androidx.annotation.VisibleForTesting
 import com.google.protobuf.ByteString
 import com.vdurmont.semver4j.Semver
@@ -82,7 +83,6 @@ import livekit.org.webrtc.RtpSender
 import livekit.org.webrtc.RtpTransceiver
 import livekit.org.webrtc.RtpTransceiver.RtpTransceiverInit
 import livekit.org.webrtc.SessionDescription
-import java.net.ConnectException
 import java.nio.ByteBuffer
 import javax.inject.Inject
 import javax.inject.Named
@@ -445,7 +445,7 @@ internal constructor(
      * reconnect Signal and PeerConnections
      */
     @Synchronized
-    @VisibleForTesting
+    @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
     fun reconnect() {
         if (reconnectingJob?.isActive == true) {
             LKLog.d { "Reconnection is already in progress" }
@@ -625,22 +625,32 @@ internal constructor(
         }
     }
 
-    internal suspend fun sendData(dataPacket: LivekitModels.DataPacket) {
-        ensurePublisherConnected(dataPacket.kind)
+    @CheckResult
+    internal suspend fun sendData(dataPacket: LivekitModels.DataPacket): Result<Unit> {
+        try {
+            ensurePublisherConnected(dataPacket.kind)
 
-        val buf = DataChannel.Buffer(
-            ByteBuffer.wrap(dataPacket.toByteArray()),
-            true,
-        )
+            val buf = DataChannel.Buffer(
+                ByteBuffer.wrap(dataPacket.toByteArray()),
+                true,
+            )
 
-        val channel = dataChannelForKind(dataPacket.kind)
-            ?: throw TrackException.PublishException("channel not established for ${dataPacket.kind.name}")
+            val channel = dataChannelForKind(dataPacket.kind)
+                ?: throw RoomException.ConnectException("channel not established for ${dataPacket.kind.name}")
 
-        channel.send(buf)
+            channel.send(buf)
+        } catch (e: Exception) {
+            return Result.failure(e)
+        }
+        return Result.success(Unit)
     }
 
     internal suspend fun waitForBufferStatusLow(kind: LivekitModels.DataPacket.Kind) {
-        ensurePublisherConnected(kind)
+        try {
+            ensurePublisherConnected(kind)
+        } catch (e: Exception) {
+            return
+        }
         val manager = when (kind) {
             LivekitModels.DataPacket.Kind.RELIABLE -> reliableDataChannelManager
             LivekitModels.DataPacket.Kind.LOSSY -> lossyDataChannelManager
@@ -650,11 +660,12 @@ internal constructor(
         }
 
         if (manager == null) {
-            throw IllegalStateException("Not connected!")
+            return
         }
         manager.waitForBufferedAmountLow(DATA_CHANNEL_LOW_THRESHOLD.toLong())
     }
 
+    @Throws(exceptionClasses = [RoomException.ConnectException::class])
     private suspend fun ensurePublisherConnected(kind: LivekitModels.DataPacket.Kind) {
         if (!isSubscriberPrimary) {
             return
@@ -671,7 +682,7 @@ internal constructor(
             this.negotiatePublisher()
         }
 
-        val targetChannel = dataChannelForKind(kind) ?: throw IllegalArgumentException("Unknown data packet kind!")
+        val targetChannel = dataChannelForKind(kind) ?: throw RoomException.ConnectException("Publisher isn't setup yet! Is room not connected?!")
         if (targetChannel.state() == DataChannel.State.OPEN) {
             return
         }
@@ -685,14 +696,14 @@ internal constructor(
             delay(50)
         }
 
-        throw ConnectException("could not establish publisher connection")
+        throw RoomException.ConnectException("could not establish publisher connection")
     }
 
     private fun dataChannelForKind(kind: LivekitModels.DataPacket.Kind) =
         when (kind) {
             LivekitModels.DataPacket.Kind.RELIABLE -> reliableDataChannel
             LivekitModels.DataPacket.Kind.LOSSY -> lossyDataChannel
-            else -> null
+            LivekitModels.DataPacket.Kind.UNRECOGNIZED -> throw IllegalArgumentException("Unknown data packet kind!")
         }
 
     private fun getPublisherOfferConstraints(): MediaConstraints {
@@ -1137,6 +1148,7 @@ internal constructor(
 
         val dataChannelInfos = LivekitModels.DataPacket.Kind.values()
             .toList()
+            .filterNot { it == LivekitModels.DataPacket.Kind.UNRECOGNIZED }
             .mapNotNull { kind -> dataChannelForKind(kind) }
             .map { dataChannel ->
                 LivekitRtc.DataChannelInfo.newBuilder()

livekit-android-sdk/src/main/java/io/livekit/android/room/datastream/StreamException.kt

@@ -17,13 +17,48 @@
 package io.livekit.android.room.datastream
 
 sealed class StreamException(message: String? = null) : Exception(message) {
+    /**
+     * Unable to open a stream with the same ID as an existing open stream.
+     */
     class AlreadyOpenedException : StreamException()
-    class AbnormalEndException(message: String?) : StreamException(message)
+
+    /**
+     * Stream closed abnormally by remote participant.
+     */
+    class AbnormalEndException(message: String? = null) : StreamException(message)
+
+    /**
+     * Incoming chunk data could not be decoded.
+     */
     class DecodeFailedException : StreamException()
+
+    /**
+     * Length exceeded total length specified in stream header.
+     */
     class LengthExceededException : StreamException()
+
+    /**
+     * Length is less than total length specified in stream header.
+     */
     class IncompleteException : StreamException()
-    class TerminatedException : StreamException()
+
+    /**
+     * Stream terminated before completion.
+     */
+    class TerminatedException(message: String? = null) : StreamException(message)
+
+    /**
+     * Cannot perform operations on an unknown stream.
+     */
     class UnknownStreamException : StreamException()
+
+    /**
+     * Given destination URL is not a directory.
+     */
     class NotDirectoryException : StreamException()
+
+    /**
+     * Unable to read information about the file to send.
+     */
     class FileInfoUnavailableException : StreamException()
 }

livekit-android-sdk/src/main/java/io/livekit/android/room/datastream/outgoing/BaseStreamSender.kt

@@ -16,21 +16,31 @@
 
 package io.livekit.android.room.datastream.outgoing
 
+import androidx.annotation.CheckResult
 import io.livekit.android.room.datastream.StreamException
 
 abstract class BaseStreamSender<T>(
     internal val destination: StreamDestination<T>,
 ) {
 
-    suspend fun write(data: T) {
+    val isOpen: Boolean
+        get() = destination.isOpen
+
+    /**
+     * Write to the stream.
+     */
+    @CheckResult
+    suspend fun write(data: T): Result<Unit> {
         if (!destination.isOpen) {
-            throw StreamException.TerminatedException()
+            return Result.failure(StreamException.TerminatedException())
         }
 
-        writeImpl(data)
+        return writeImpl(data)
     }
 
-    internal abstract suspend fun writeImpl(data: T)
+    @CheckResult
+    internal abstract suspend fun writeImpl(data: T): Result<Unit>
+
     suspend fun close(reason: String? = null) {
         destination.close(reason)
     }
@@ -41,7 +51,9 @@ abstract class BaseStreamSender<T>(
  */
 interface StreamDestination<T> {
     val isOpen: Boolean
-    suspend fun write(data: T, chunker: DataChunker<T>)
+
+    @CheckResult
+    suspend fun write(data: T, chunker: DataChunker<T>): Result<Unit>
     suspend fun close(reason: String?)
 }
 

livekit-android-sdk/src/main/java/io/livekit/android/room/datastream/outgoing/ByteStreamSender.kt

@@ -30,8 +30,9 @@ class ByteStreamSender(
     val info: ByteStreamInfo,
     destination: StreamDestination<ByteArray>,
 ) : BaseStreamSender<ByteArray>(destination = destination) {
-    override suspend fun writeImpl(data: ByteArray) {
-        destination.write(data, byteDataChunker)
+
+    override suspend fun writeImpl(data: ByteArray): Result<Unit> {
+        return destination.write(data, byteDataChunker)
     }
 }
 
@@ -50,9 +51,7 @@ private val byteDataChunker: DataChunker<ByteArray> = { data: ByteArray, chunkSi
 }
 
 /**
- * Reads the file and writes it to the data stream.
- *
- * @throws
+ * Reads the file from [filePath] and writes it to the data stream.
  */
 suspend fun ByteStreamSender.writeFile(filePath: String) {
     write(FileSystem.SYSTEM.source(filePath.toPath()))
@@ -68,14 +67,23 @@ suspend fun ByteStreamSender.write(input: InputStream) {
 /**
  * Reads the source and sends it to the data stream.
  */
-suspend fun ByteStreamSender.write(source: Source) {
+suspend fun ByteStreamSender.write(source: Source): Result<Unit> {
     val buffer = Buffer()
     while (true) {
-        val readLen = source.read(buffer, 4096)
-        if (readLen == -1L) {
-            break
-        }
+        try {
+            val readLen = source.read(buffer, 4096)
+            if (readLen == -1L) {
+                break
+            }
 
-        write(buffer.readByteArray())
+            val result = write(buffer.readByteArray())
+            if (result.isFailure) {
+                return result
+            }
+        } catch (e: Exception) {
+            return Result.failure(e)
+        }
     }
+
+    return Result.success(Unit)
 }