Add ECHO virtual channel support across IronRDP#1109
Add ECHO virtual channel support across IronRDP#1109Benoît Cortier (CBenoit) merged 1 commit intomasterfrom
Conversation
Implement spec-minimal MS-RDPEECO support with feature-gated client/server integration and runtime RTT probing. - add new ironrdp-echo crate with ECHO PDUs and client/server DVC processors - wire echo feature through ironrdp meta crate, client, and server - add server-side echo handle/bridge and dynamic channel lookup helper - add end-to-end echo virtual channel test in testsuite-extra - update server docs and fix fmt/clippy/doctest issues for CI parity
Coverage Report 🤖 ⚙️Past: New: Diff: +1.38% [this comment will be updated automatically] |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive ECHO virtual channel support across IronRDP, implementing the MS-RDPEECO protocol for round-trip time (RTT) measurement between RDP client and server.
Changes:
- Introduces new
ironrdp-echocrate with PDU encoding/decoding for ECHO request and response messages - Adds feature-gated ECHO support to
ironrdp,ironrdp-client, andironrdp-servercrates - Implements server-side RTT tracking via
EchoServerHandlewith thread-safe measurement collection - Provides end-to-end integration test demonstrating ECHO channel functionality
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-echo/src/pdu.rs | PDU structures for ECHO_REQUEST_PDU and ECHO_RESPONSE_PDU per MS-RDPEECO spec |
| crates/ironrdp-echo/src/client.rs | Client-side DVC processor that echoes requests back as responses |
| crates/ironrdp-echo/src/server.rs | Server-side DVC processor for sending requests and receiving responses |
| crates/ironrdp-echo/src/lib.rs | Library entry point with module exports and channel name constant |
| crates/ironrdp-echo/tests/echo.rs | Integration tests for PDU encoding, client/server behavior |
| crates/ironrdp-echo/Cargo.toml | Package configuration for the new crate |
| crates/ironrdp-echo/README.md | Documentation describing the ECHO extension implementation |
| crates/ironrdp-echo/CHANGELOG.md | Changelog file for version tracking |
| crates/ironrdp-server/src/echo.rs | Server-side ECHO handle with RTT measurement tracking and DVC bridge |
| crates/ironrdp-server/src/server.rs | Integration of ECHO channel into server event loop and DVC setup |
| crates/ironrdp-server/src/lib.rs | Export of ECHO-related types when feature is enabled |
| crates/ironrdp-server/README.md | Documentation for using ECHO RTT probes in server applications |
| crates/ironrdp-server/Cargo.toml | Added ironrdp-echo dependency with echo feature flag |
| crates/ironrdp-client/src/rdp.rs | Registration of EchoClient in both TCP and WebSocket connection paths |
| crates/ironrdp-client/Cargo.toml | Added echo feature flag that propagates to ironrdp crate |
| crates/ironrdp/src/lib.rs | Re-export of ironrdp-echo crate when echo feature is enabled |
| crates/ironrdp/Cargo.toml | Added echo feature and ironrdp-echo dependency |
| crates/ironrdp-dvc/src/server.rs | New method to get DVC channel ID by processor type |
| crates/ironrdp-testsuite-extra/tests/mod.rs | End-to-end test for ECHO virtual channel with RTT measurement |
| crates/ironrdp-testsuite-extra/Cargo.toml | Enabled dvc and echo features for integration testing |
| Cargo.lock | Updated with ironrdp-echo package entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| native-tls = ["ironrdp-tls/native-tls", "tokio-tungstenite/native-tls", "ironrdp-mstsgu/native-tls"] | ||
| qoi = ["ironrdp/qoi"] | ||
| qoiz = ["ironrdp/qoiz"] | ||
| echo = ["ironrdp/echo"] |
There was a problem hiding this comment.
question: Do we really want to put that behind a feature flag?
| if channel.state != ChannelState::Opened || !channel.processor.as_any().is::<T>() { | ||
| return None; | ||
| } | ||
|
|
||
| id.try_into().ok() |
There was a problem hiding this comment.
note: This does not follow any existing pattern and can be confusing. I’ll send a follow up.
There was a problem hiding this comment.
issue: Tests should go into a testsuite crate.
| ironrdp-core = { path = "../ironrdp-core", version = "0.1" } # public | ||
| ironrdp-dvc = { path = "../ironrdp-dvc", version = "0.5" } # public | ||
| ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.7" } # public |
There was a problem hiding this comment.
note: I didn’t check, but # public should be used only for dependencies whose types are used in the public API
| #[cfg(feature = "echo")] | ||
| mod echo; |
There was a problem hiding this comment.
thought: I think there is a better way to keep the crate modularized that is not using feature flags. We should look into this before adding too much of these.
Summary
Validation