Improve WebRTC data channel iteration, exceptions#5684
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR introduces a unified exception hierarchy for WebRTC data channels (WebRtc.IOException and WebRtc.DataChannelClosedException), adds an iterator method for message consumption, implements a withIOException helper for exception wrapping, adds requireOpen() validation guards across all platform implementations (Android, iOS, Rust, Web), and includes comprehensive tests for iterator behavior and closed-channel error handling. ChangesWebRTC Data Channel Error Handling and Iterator Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt (1)
247-256: ⚡ Quick winMake wrapped I/O errors include operation context.
withIOExceptionalways throws"Error in WebRtcDataChannel operation", which drops useful context (e.g., text vs binary send, channel label) at call sites. Consider accepting a context message parameter and propagating it into the wrapped exception.Suggested diff
-@InternalAPI -public suspend inline fun <R> withIOException(crossinline block: suspend () -> R): R { +@InternalAPI +public suspend inline fun <R> withIOException( + operation: String, + crossinline block: suspend () -> R +): R { return try { block() } catch (cause: kotlinx.coroutines.CancellationException) { throw cause } catch (cause: WebRtc.IOException) { throw cause } catch (cause: Exception) { - throw WebRtc.IOException("Error in WebRtcDataChannel operation", cause) + throw WebRtc.IOException("Error in $operation", cause) } }As per coding guidelines, error messages should be actionable and include problematic context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt` around lines 247 - 256, withIOException currently replaces all inner error context with the generic string "Error in WebRtcDataChannel operation"; change its signature to accept a context message parameter (e.g., context: String = "WebRtcDataChannel operation") and use that context when wrapping non-Cancellation/IOException exceptions into WebRtc.IOException so the thrown WebRtc.IOException includes the operation-specific message; then update all callers inside WebRtcDataChannel (e.g., send/sendText/sendBinary/any methods that call withIOException) to pass a descriptive context like "send text (label=...)" or "send binary (label=...)" so logs and stacktraces preserve actionable context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.kt`:
- Around line 247-256: withIOException currently replaces all inner error
context with the generic string "Error in WebRtcDataChannel operation"; change
its signature to accept a context message parameter (e.g., context: String =
"WebRtcDataChannel operation") and use that context when wrapping
non-Cancellation/IOException exceptions into WebRtc.IOException so the thrown
WebRtc.IOException includes the operation-specific message; then update all
callers inside WebRtcDataChannel (e.g., send/sendText/sendBinary/any methods
that call withIOException) to pass a descriptive context like "send text
(label=...)" or "send binary (label=...)" so logs and stacktraces preserve
actionable context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b136aef9-d915-4e6b-8b6a-39e60f3a4876
📒 Files selected for processing (10)
ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.ktktor-client/ktor-client-webrtc/api/jvm/ktor-client-webrtc.apiktor-client/ktor-client-webrtc/api/ktor-client-webrtc.klib.apiktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtc.ktktor-client/ktor-client-webrtc/common/src/io/ktor/client/webrtc/WebRtcDataChannel.ktktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRtcDataChannelIteratorTest.ktktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRtcDataChannelTest.ktktor-client/ktor-client-webrtc/ios/src/io/ktor/client/webrtc/DataChannel.ktktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/common/src/io/ktor/client/webrtc/rs/DataChannel.ktktor-client/ktor-client-webrtc/web/src/io/ktor/client/webrtc/DataChannel.kt
d46e999 to
2eed7b1
Compare
…l iterator # Conflicts: # ktor-client/ktor-client-webrtc/android/src/io/ktor/client/webrtc/DataChannel.kt # ktor-client/ktor-client-webrtc/common/test/io/ktor/client/webrtc/WebRtcDataChannelTest.kt
2eed7b1 to
605f2c8
Compare
Subsystem
WebRTC Client
Motivation
KTOR-9610 WebRtcPeerConnection::close causes ClosedReceiveChannelException for other peer
Solution
ReceiveChannel.receive()throws a general closed channel exception with a stacktrace pointing atclose(), which is confusing for users; throw WebRtc.ClosedException insteadWebRtc.IOExceptioninWebRtc.Datachannel.send()so users can catch it in common code