Repository navigation
fix: drop connection on pion event that PeerConnectionState has changed to disconnected, closed, or failed - #194
Conversation
WalkthroughTop-level WebRTC tasks now construct PeerConnection then forward explicit event/command channels into a new inner task. New Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-08T08:41:38.069ZApplied to files:
🧬 Code graph analysis (2)crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (2)
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 |
3cf9930 to
9df7d59
Compare
9df7d59 to
053b31a
Compare
|
CI static checks are failing because CI is running a different version of rust than rust-toolchain.toml. Addressing that in #195 |
053b31a to
9f978ab
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
328-417: Consider simplifying test implementation.The tests correctly verify terminal state handling, but could be improved:
Line 371: As noted in past comments, there's a potential race between sending the Connected event and checking
is_finished(). Consider adding a shorttokio::time::sleep()ortokio::task::yield_now().awaitto ensure the event is processed before the assertion.Lines 378-391: The polling loop checking
is_finished()every 100ms is more complex than needed. Consider simplifying:Apply this diff to simplify the timeout logic:
+ // Give task a moment to process the Connected state + tokio::task::yield_now().await; + assert!(!task_handle.is_finished()); // Send new state that we expect to end the task peer_evt_send .send(tx5_go_pion::PeerConnectionEvent::State(state)) .unwrap(); - tokio::time::timeout(std::time::Duration::from_secs(5), async move { - loop { - if task_handle.is_finished() { - assert!(task_handle.await.unwrap().is_err()); - - // Test passes, the task finished with an error - break; - } - - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - } - }) - .await - .expect("Timed out"); + let result = tokio::time::timeout( + std::time::Duration::from_secs(5), + task_handle + ) + .await + .expect("Timed out waiting for task to finish"); + + assert!(result.unwrap().is_err(), "Task should finish with an error");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (1)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (2)
config(244-249)new(104-168)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Test (ubuntu-latest, stable)
🔇 Additional comments (4)
crates/tx5-connection/src/webrtc/go_pion.rs (4)
19-19: LGTM: Error variant addition is appropriate.The new
Errorvariant enables propagation of connection errors through the command channel, which is essential for the new error handling logic.
107-127: LGTM: Refactoring enables testability.Moving
PeerConnectioncreation intotaskand passing it totask_inneris a clean separation that enables the tests to inject controlled events while keeping production behavior unchanged.
157-182: LGTM: Terminal state handling is correct.The logic correctly identifies and handles the three terminal PeerConnectionState values (Disconnected, Closed, Failed) by forwarding them as errors, which aligns with the WebRTC spec and matches the libdatachannel backend behavior.
282-282: LGTM: Error propagation is correct.The
Cmd::Errorhandling properly exits the main loop with the contained error, ensuring the task terminates when terminal states are detected.
9f978ab to
0e3fc26
Compare
af920da to
6752c79
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
328-416: Tests validate the core functionality.The tests correctly verify that terminal PeerConnectionState transitions (Disconnected, Closed, Failed) cause the task to finish with an error. The 5-second timeout prevents hanging, and the test structure is reasonable.
Optional improvement: The busy-wait pattern (lines 378-387) could be replaced with a more deterministic approach. Consider using a dedicated signal or channel to indicate when the task has processed the state change, rather than polling
is_finished()every 100ms. However, the current implementation is functional and acceptable for test code.For example, you could send a message through the data channel after the Connected state and verify it's received, then send the terminal state and verify the task completes. This would be more aligned with the earlier review suggestion to "demonstrate that the task is still functioning."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tx5-connection/src/webrtc/libdatachannel.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(244-249)new(104-168)task_inner(229-415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (windows-latest, stable)
🔇 Additional comments (5)
crates/tx5-connection/src/webrtc/go_pion.rs (5)
19-19: LGTM! Error variant correctly added.The
Errorvariant addition to theCmdenum aligns with the libdatachannel implementation and enables proper error propagation from the event handlers.
107-127: LGTM! Clean refactoring to enable event handling.The separation of PeerConnection creation into
taskand logic handling intotask_inneris a good design that enables proper event stream forwarding and test access to the PeerConnection.
129-139: LGTM! Signature correctly updated.The
task_innersignature changes properly accept thePeerConnectionand its event receiver, enabling the new event-handling flow.
150-182: Implementation correctly handles terminal states.The event handling properly:
- Forwards
Evt::ErrorasCmd::Errorwith properly formatted message- Converts terminal PeerConnectionState variants (Disconnected, Closed, Failed) to
Cmd::Error, causing task termination- Aligns with the libdatachannel backend behavior
This addresses the PR objective to drop connections on pion events for these terminal states per the WebRTC spec.
Note: Past review comments discussed whether terminal states should be treated as errors versus graceful termination. This was appropriately deferred to a separate refactor that may span both backends.
282-282: LGTM! Error propagation is correct.The
Cmd::Errorhandling properly propagates errors to terminate the task, completing the error-handling flow from the event handlers.
jost-s
left a comment
jost-s
left a comment
There was a problem hiding this comment.
Reviewed this and looks good to me
neonphog
left a comment
neonphog
left a comment
There was a problem hiding this comment.
This looks fine to me. The test isn't really testing much... just that the event gets re-published. But it's probably the best you can do for a unit test. It'd be good to add some integration tests of this at some point.
1102229 to
59daf3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(5 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
task_inner(130-277)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-417)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (macos-latest, stable)
ThetaSinner
left a comment
ThetaSinner
left a comment
There was a problem hiding this comment.
Build seems to be broken, and why is ConnCmd also now changing? Is that required because of changing events in the webrtc module?
59daf3d to
e0c9122
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(5 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tx5-connection/src/webrtc/libdatachannel.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (1)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-417)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (macos-latest, stable)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
e0c9122 to
82e8103
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
301-305: Data channel events could propagate closure more explicitly.When the data channel encounters
ErrororCloseevents, the spawned task simply breaks without notifying the main task. While this is consistent with the libdatachannel implementation, you could consider sendingCmd::CloseorCmd::Errorto propagate these events more explicitly to the main loop.If you want to improve observability, consider:
match evt { - Evt::Error(_) => break, + Evt::Error(err) => { + cmd_send.send_or_close(Cmd::Error( + std::io::Error::other(format!("Data channel error: {err}")) + ))?; + break; + } Evt::Open => { cmd_send.send_or_close(Cmd::DataChanOpen)?; } - Evt::Close => break, + Evt::Close => { + cmd_send.send_or_close(Cmd::Close)?; + break; + }
372-382: Simplify the timeout logic.The polling loop can be replaced with a direct timeout on the task handle.
Apply this diff to simplify:
- let result = tokio::time::timeout(std::time::Duration::from_secs(5), async move { - loop { - if task_handle.is_finished() { - return task_handle.await.unwrap(); - } - - tokio::time::sleep(std::time::Duration::from_millis(100)).await; - } - }) - .await - .expect("Timed out"); + let result = tokio::time::timeout( + std::time::Duration::from_secs(5), + task_handle + ) + .await + .expect("Timed out waiting for task to finish") + .expect("Task panicked");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(5 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/tx5-connection/src/webrtc/libdatachannel.rs
- crates/tx5-connection/src/webrtc.rs
- crates/tx5-connection/src/conn.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (1)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-417)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
🔇 Additional comments (5)
crates/tx5-connection/src/webrtc/go_pion.rs (5)
19-20: LGTM! New command variants properly distinguish normal closure from errors.The
CloseandErrorvariants align with the libdatachannel implementation and correctly model the distinction between graceful termination and error states.
108-128: LGTM! Clean refactoring that enables testability.Moving
PeerConnectioncreation to the outertaskfunction and delegating totask_innerallows tests to inject mock events while keeping the core logic intact.
151-173: LGTM! State transitions correctly align with WebRTC spec.The handling properly distinguishes normal termination states (
Disconnected,Closed→Cmd::Close) from error states (Failed,Error→Cmd::Error), matching the W3C WebRTC spec and the libdatachannel implementation.
273-277: LGTM! Commands properly terminate the task.
Cmd::Closenow correctly breaks the loop after emittingWebrtcEvt::Closed, ensuring the task terminates promptly in normal shutdown scenarios.Cmd::Errorpropagates errors appropriately.
387-403: Excellent test coverage of state transitions.The tests correctly verify that
ClosedandDisconnectedstates terminate gracefully (Ok), whileFailedpropagates an error (Err), matching the WebRTC spec and PR objectives.
82e8103 to
e02a0c7
Compare
Not required, just seemed liker clearer implementation than having a no-op in the match arm of I don't feel strongly if you prefer that implementation. |
e84fcea to
a7e4447
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/tx5-connection/src/conn.rs (2)
615-622: Close the semaphore before breaking on WebRTC closure.When
WebrtcRecv(Closed)is received, the loop breaks without closingtask_core.ready, leaving any callers ofConn::ready()hanging indefinitely. The semaphore must be closed before the break to unblock waiters.Apply this diff:
WebrtcRecv(Closed) => { netaudit!( DEBUG, pub_key = ?task_core.pub_key, a = "webrtc processing task closed", ); + task_core.ready.close(); break; }
654-660: Close the semaphore before breaking on WebRTC error.When
WebrtcErroris received, the loop breaks without closingtask_core.ready, leaving any callers ofConn::ready()hanging indefinitely. The semaphore must be closed before the break to unblock waiters.Apply this diff:
WebrtcError => { netaudit!( WARN, pub_key = ?task_core.pub_key, a = "webrtc processing task closed due to error", ); + task_core.ready.close(); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(5 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tx5-connection/src/webrtc.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/conn.rscrates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (2)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-417)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
task_inner(130-282)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: Test (windows-latest, stable)
- GitHub Check: Test (ubuntu-latest, stable)
- GitHub Check: Test (macos-latest, stable)
🔇 Additional comments (5)
crates/tx5-connection/src/conn.rs (1)
378-395: Well-structured close and error handling.The explicit matching on
WebrtcEvt::Closedwith a dedicated log message and break path cleanly separates normal closure from error scenarios. This makes the control flow easier to follow and debug.crates/tx5-connection/src/webrtc/go_pion.rs (4)
151-173: Proper alignment with WebRTC PeerConnectionState spec.The mapping of PeerConnectionState transitions to internal commands correctly implements the terminal states per the W3C specification:
DisconnectedandClosed→Cmd::Close(graceful closure)Failed→Cmd::Error(error condition)This matches the libdatachannel backend behavior and ensures consistent connection lifecycle handling across backends.
273-277: Correct task termination on close and error.The
Cmd::Closehandler properly emitsWebrtcEvt::Closedand breaks the loop to returnOk(()), whileCmd::Errorimmediately propagates the error. This ensures the task terminates promptly in both scenarios without leaving resources hanging.
327-386: Verify test reliability without waiting.The helper function polls
task_handle.is_finished()in a loop with 100ms sleeps. This approach may be flaky on slower CI machines and doesn't definitively prove the task terminated due to the state change (the task could finish for other reasons after the sleep).Consider using a more deterministic approach:
// Instead of polling is_finished, await the handle directly with timeout let result = tokio::time::timeout( std::time::Duration::from_secs(5), task_handle ) .await .expect("Task did not complete within timeout");This eliminates the polling loop and more directly validates that the task completes within the timeout window.
388-414: Good test coverage of terminal state transitions.The three test cases comprehensively validate that:
ClosedandDisconnectedstates complete withOk(graceful termination)Failedstate completes withErr(error termination)This ensures the implementation correctly distinguishes between clean closure and error conditions as specified in the WebRTC spec.
5f23c06 to
f2bbd3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/tx5-connection/src/conn.rs (1)
638-645: Close the semaphore before breaking.When
WebrtcClosedarrives beforeReady, the semaphore is never closed andConn::ready()waiters hang indefinitely. This is the same issue flagged in previous reviews (though at different line numbers due to refactoring).Apply this diff to close the semaphore before breaking:
WebrtcClosed => { netaudit!( WARN, pub_key = ?task_core.pub_key, a = "webrtc processing task closed", ); + task_core.ready.close(); break; }
🧹 Nitpick comments (1)
crates/tx5-connection/src/conn.rs (1)
604-606: Clarify the purpose of this no-op arm.Given that
webrtc_taskbreaks onClosed(line 378) before forwarding the event, this match arm appears unreachable. If this is defensive programming or future-proofing, consider adding a comment explaining when this code path would be executed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(2 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tx5-connection/src/webrtc.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/conn.rscrates/tx5-connection/src/webrtc/libdatachannel.rscrates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
task_inner(130-282)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-418)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: Test (macos-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (10)
crates/tx5-connection/src/conn.rs (1)
377-385: Early break on Closed event is correct.The logic correctly breaks before forwarding the Closed event and then sends WebrtcClosed to signal task completion. This prevents Closed from being processed in the main command loop while ensuring cleanup notification.
crates/tx5-connection/src/webrtc/libdatachannel.rs (4)
22-23: Close variant correctly added.Adding
Closeto theCmdenum enables explicit close signaling, matching the go_pion backend implementation and allowing graceful shutdown propagation.
34-36: Correctly treats datachannel close as normal termination.Changing from
Cmd::ErrortoCmd::Closeproperly distinguishes between error states and normal connection closure, aligning with WebRTC PeerConnectionState semantics.
220-228: Function rename improves consistency.Renaming
task_errtotask_inneraligns the naming convention with the go_pion backend, making the codebase more consistent and easier to navigate.
409-412: Close handling is correct.The task properly sends
WebrtcEvt::Closedand terminates the loop, ensuring clean shutdown when the datachannel closes. The break statement (previously missing) is now correctly in place.crates/tx5-connection/src/webrtc/go_pion.rs (5)
19-20: Cmd enum correctly extended.Adding
CloseandErrorvariants enables proper distinction between normal termination and error conditions, aligning both backends' behavior with the WebRTC PeerConnectionState specification.
108-128: Refactoring enables better testing.Separating
PeerConnectioncreation from the task logic allows tests to inject controlled events through thepeer_evtchannel, improving testability without affecting production behavior.
151-173: PeerConnectionState handling correctly aligns with WebRTC spec.The state transitions follow the W3C WebRTC specification:
DisconnectedandClosedtriggerCmd::Close(normal termination)FailedtriggersCmd::Error(abnormal termination)This semantic distinction ensures proper error logging and avoids misleading "WebRTC task failed" messages for graceful shutdowns.
273-277: Close and Error handling is correct.
Cmd::Close: SendsWebrtcEvt::Closedand breaks, returningOk(())to indicate graceful shutdownCmd::Error: ReturnsErrto indicate abnormal terminationThis distinction ensures proper error logging—only actual errors trigger the "WebRTC task failed" message at line 186.
323-415: Tests effectively verify state-driven termination.The test suite correctly validates:
ClosedandDisconnectedstates terminate withOk(())(graceful)Failedstate terminates withErr(error)The polling approach (100ms intervals, 5s timeout) is pragmatic for testing async task completion.
Based on learnings
|
I see, it's part of propogating the close event up. Okay. Got it |
f2bbd3f to
a22478f
Compare
|
This is making the recently added integration test of reconnection to flake inconsistently on both go-pion and libdatachannel backends, so will need to investigate that. It flakes on Flakyness is resolved by #199 |
a22478f to
418143e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
331-390: Consider simplifying the test polling loop.The tests manually poll
task_handle.is_finished()with sleep intervals, but you can simplify by directly awaiting the handle within the timeout:let result = tokio::time::timeout( std::time::Duration::from_secs(5), task_handle ) .await .expect("Timed out") .unwrap();This eliminates the polling loop while achieving the same result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(2 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/conn.rscrates/tx5-connection/src/webrtc/go_pion.rscrates/tx5-connection/src/webrtc/libdatachannel.rs
🧬 Code graph analysis (2)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-418)
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
task_inner(130-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (macos-latest)
🔇 Additional comments (12)
crates/tx5-connection/src/webrtc.rs (1)
12-12: LGTM! Clean addition of the Closed variant.The new
Closedvariant properly represents terminal connection states and integrates well with the event flow implemented in both backends.crates/tx5-connection/src/conn.rs (2)
377-379: LGTM! Proper early termination on connection closure.Breaking early when receiving
WebrtcEvt::Closedprevents forwarding subsequent events and ensures the webrtc task exits cleanly. This aligns with the terminal state semantics.
604-606: LGTM! Correct handling during negotiation phase.Ignoring
Closedevents during the WebRTC negotiation phase prevents premature fallback to signal relay. This is appropriate since the connection may legitimately close during setup without requiring fallback behavior.crates/tx5-connection/src/webrtc/go_pion.rs (5)
19-20: LGTM! Appropriate new command variants.The
CloseandError(std::io::Error)variants properly distinguish between expected connection closure and unexpected errors, aligning with the WebRTC PeerConnectionState spec.
108-128: LGTM! Smart refactoring for testability.Moving PeerConnection creation to the outer
taskfunction and forwarding event channels totask_innerenables unit testing by allowing synthetic event injection, as demonstrated in the tests below.
151-177: LGTM! Correct state mapping per WebRTC spec.The event forwarding correctly implements the WebRTC PeerConnectionState spec:
DisconnectedandClosedmap toCmd::Close(expected terminal states)FailedandErrorevents map toCmd::Error(unexpected failures)All branches properly break after sending commands to terminate the peer event loop.
277-281: LGTM! Proper task termination handling.The command handling correctly terminates the inner task:
Cmd::CloseemitsWebrtcEvt::Closedand breaks cleanly, whileCmd::Errorpropagates the error. This ensures timely task shutdown on connection closure.
392-418: LGTM! Comprehensive test coverage for state transitions.The three tests verify that terminal PeerConnectionState transitions correctly finish the task with the expected result:
Ok(())forClosedandDisconnected,Err(_)forFailed. This validates the spec-compliant behavior.crates/tx5-connection/src/webrtc/libdatachannel.rs (4)
22-22: LGTM! Consistent with go_pion backend.The
Cmd::Closevariant addition aligns the libdatachannel backend with the go_pion implementation, ensuring both backends handle expected connection closure consistently.
35-35: LGTM! Proper mapping of expected closure.Changing
on_closedto sendCmd::Closecorrectly treats data channel closure as an expected event rather than an error, matching the semantics established in the go_pion backend.
221-221: LGTM! Clearer function naming.Renaming from
task_errtotask_innerbetter reflects the function's purpose and matches the naming in go_pion.rs, improving consistency across backends.Also applies to: 228-228
409-412: LGTM! Correct task termination on close.The
Cmd::Closehandler properly emitsWebrtcEvt::Closedand breaks the loop, ensuring the task terminates cleanly. This matches the go_pion implementation and addresses the previous review concern.
418143e to
6b5cc4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
373-387: Simplify the polling loop.The manual polling loop can be replaced with a direct
awaiton the task handle, which is cleaner and avoids unnecessary sleep iterations.Apply this diff:
let result = tokio::time::timeout( std::time::Duration::from_secs(5), - async move { - loop { - if task_handle.is_finished() { - return task_handle.await.unwrap(); - } - - tokio::time::sleep(std::time::Duration::from_millis(100)) - .await; - } - }, + task_handle, ) .await - .expect("Timed out"); + .expect("Timed out") + .expect("Task panicked"); result
331-390: Consider verifying event emission in tests.The tests verify task termination but don't confirm that
WebrtcEvt::Closedis actually emitted forClosestates. While the current tests are adequate, receiving from_evt_recvwould provide stronger verification.Example enhancement:
let (evt_send, mut evt_recv) = CloseSend::sized_channel(1024); // ... after sending terminal state ... // For Close states, verify Closed event is emitted if matches!(state, PeerConnectionState::Closed | PeerConnectionState::Disconnected) { match tokio::time::timeout(Duration::from_secs(1), evt_recv.recv()).await { Ok(Some(WebrtcEvt::Closed)) => {}, other => panic!("Expected WebrtcEvt::Closed, got {:?}", other), } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(2 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/tx5-connection/src/webrtc.rs
- crates/tx5-connection/src/conn.rs
- crates/tx5-connection/src/webrtc/libdatachannel.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (1)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-418)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (macos-latest)
- GitHub Check: test (x86_64, 33, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 33, 26.0.10792818, go-pion)
- GitHub Check: test (x86_64, 29, 26.0.10792818, datachannel)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
🔇 Additional comments (5)
crates/tx5-connection/src/webrtc/go_pion.rs (5)
19-20: LGTM! Cmd variants enable proper state differentiation.The new
CloseandErrorvariants allow the task to distinguish between clean peer disconnections and actual failures, aligning with the libdatachannel implementation.
108-128: LGTM! Task split enables testability.The separation of
task(creation) andtask_inner(logic) enables unit testing by allowing injection of controlled peer events, as demonstrated in the test module.
130-140: LGTM! Signature supports testable design.The additional
peerandpeer_evtparameters enable controlled testing while maintaining production behavior.
277-281: LGTM! Termination logic is correct.Both handlers properly terminate the task:
Closesignals clean shutdown viaWebrtcEvt::Closedthen breaks, whileErrorimmediately propagates the error upward.
159-163: No changes needed—the implementation is correct as tested.The code deliberately treats
Disconnectedas a terminal state and breaks the event loop. This behavior is explicitly validated by the testconn_dropped_on_peer_connection_state_disconnected_and_returns_ok(), which expectsOk()whenDisconnectedis triggered. The codebase contains no reconnection or recovery logic, confirming the connection model is single-use, not long-lived with auto-reconnect. Both the pion and libdatachannel backends follow this same pattern: terminal peer connection states trigger loop termination. The design is intentional and consistent across backends.Likely an incorrect or invalid review comment.
9497057 to
b0423bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
373-387: Consider simplifying the task completion check.The current polling loop works correctly but could be simplified by directly awaiting the task handle with a timeout:
let result = tokio::time::timeout( std::time::Duration::from_secs(5), task_handle, ) .await .expect("Timed out") .unwrap();This achieves the same result with less code and without busy-waiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/tx5-connection/src/conn.rs(2 hunks)crates/tx5-connection/src/webrtc.rs(1 hunks)crates/tx5-connection/src/webrtc/go_pion.rs(5 hunks)crates/tx5-connection/src/webrtc/libdatachannel.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/tx5-connection/src/conn.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
📚 Learning: 2025-08-08T08:41:38.069Z
Learnt from: ThetaSinner
Repo: holochain/tx5 PR: 167
File: crates/tx5/tests/tests/flaky_sig.rs:73-84
Timestamp: 2025-08-08T08:41:38.069Z
Learning: Repo holochain/tx5: In test code (crates/tx5/tests/tests/flaky_sig.rs), maintainer (ThetaSinner) prefers not to refactor non-ideal async patterns; leaving block_on in Drop for FlakyRelay is acceptable. Treat similar test-only cleanup suggestions as non-blocking unless they cause flakes/panics.
Applied to files:
crates/tx5-connection/src/webrtc/libdatachannel.rscrates/tx5-connection/src/webrtc/go_pion.rs
🧬 Code graph analysis (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
crates/tx5-connection/src/webrtc/go_pion.rs (1)
task_inner(130-286)
crates/tx5-connection/src/webrtc/go_pion.rs (2)
crates/tx5-connection/src/webrtc/libdatachannel.rs (3)
config(243-248)new(103-167)task_inner(228-418)crates/tx5-connection/src/lib.rs (1)
sized_channel(77-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (x86_64, 29, 26.0.10792818, go-pion)
- GitHub Check: Test (macos-latest)
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (6)
crates/tx5-connection/src/webrtc.rs (1)
12-12: LGTM! Clean addition of the Closed variant.The
Closedvariant appropriately extends the event enum to signal graceful WebRTC connection closure, aligning both backends (go-pion and libdatachannel) with the WebRTC spec.crates/tx5-connection/src/webrtc/go_pion.rs (4)
19-20: Well-designed distinction between graceful closure and errors.Adding separate
CloseandErrorvariants properly distinguishes terminal states that are part of normal operation (disconnected/closed) from genuine failures. This addresses the concern raised in past reviews about misleading error logs.
108-128: LGTM! Clean refactoring for testability.Extracting peer connection initialization into the outer
taskfunction and delegating event handling totask_innerenables unit testing of state transition logic without full peer connection integration. The separation is clean and maintains clarity.
151-177: Correct implementation of PeerConnectionState handling per W3C spec.The state transition logic properly maps peer connection states to close/error commands:
DisconnectedandClosed→Cmd::Close(graceful termination)Failed→Cmd::Error(error termination)Errorevent →Cmd::ErrorThis aligns with the W3C RTCPeerConnectionState spec and matches the libdatachannel backend behavior as stated in the PR objectives.
277-281: Proper command handling for Close and Error.The implementation correctly:
- Emits
WebrtcEvt::Closedupstream before gracefully terminating the loop forCmd::Close- Propagates errors immediately for
Cmd::ErrorThis matches the agreed design from past review discussions and ensures graceful shutdowns don't log misleading error messages.
crates/tx5-connection/src/webrtc/libdatachannel.rs (1)
22-22: LGTM! Libdatachannel backend now aligns with go-pion.The changes successfully align the libdatachannel backend with the go-pion implementation:
- Line 22:
Cmd::Closevariant added to support graceful closure signaling- Line 35: Data channel
on_closednow sendsCmd::Closeinstead of treating closure as an error- Lines 409-412:
Cmd::Closehandling emitsWebrtcEvt::Closedand breaks the loop (past review confirmed thebreakis essential to terminate the task)This ensures both backends handle connection closure consistently and emit the appropriate events upstream.
Also applies to: 35-35, 409-412
… events that PeerConnectionState has changed to disconnected, closed, or failed
b0423bf to
03de1dd
Compare
|
✔️ 03de1dd - Conventional commits check succeeded. |
5 participants
With the pion backend, connections are now closed upon receiving an event that the
RTCPeerConnectionstate has changed todisconnect,closedorfailed.The spec outlines these states: https://w3c.github.io/webrtc-pc/#rtcpeerconnectionstate-enum. I believe this is the right approach for handling them. This is how it is already implemented in the libdatachannel backend.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Public API
Tests