feat(server): expose NetworkAutoDetect RTT via a shared handle#1346
Open
Greg Lamberson (glamberson) wants to merge 1 commit into
Open
feat(server): expose NetworkAutoDetect RTT via a shared handle#1346Greg Lamberson (glamberson) wants to merge 1 commit into
Greg Lamberson (glamberson) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes the server’s NetworkAutoDetect RTT measurement via a shared Arc<AtomicU32> handle so display backends can read a fresh RTT value even after run() takes ownership of the server.
Changes:
- Add
autodetect_rtt: Arc<AtomicU32>toRdpServer, plus anautodetect_rtt_handle()getter and builder injectionwith_autodetect_rtt_handle(...). - Update the
AutoDetectRsphandler to store the latest measuredrtt_msinto the shared atomic handle. - Add tests to cover the
u32::MAXsentinel default and builder handle injection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crates/ironrdp-server/src/server.rs |
Adds the shared RTT handle to RdpServer, exposes it via an accessor, and updates it on auto-detect responses. |
crates/ironrdp-server/src/builder.rs |
Extends the builder to allow injecting a shared RTT handle used by both server and backend. |
crates/ironrdp-testsuite-core/tests/server/autodetect.rs |
Adds tests for the new RTT handle API and sentinel behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
391
to
395
| autodetect: None, | ||
| connection_handler, | ||
| display_suppressed: display_suppressed.unwrap_or_else(|| Arc::new(AtomicBool::new(false))), | ||
| autodetect_rtt: autodetect_rtt.unwrap_or_else(|| Arc::new(AtomicU32::new(u32::MAX))), | ||
| } |
There was a problem hiding this comment.
True, we could load the sentinel RTT when the user provides its own handle
Comment on lines
+105
to
+116
| let handle = Arc::new(AtomicU32::new(42)); | ||
| let server = RdpServer::builder() | ||
| .with_addr(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))) | ||
| .with_no_security() | ||
| .with_no_input() | ||
| .with_no_display() | ||
| .with_autodetect_rtt_handle(Arc::clone(&handle)) | ||
| .build(); | ||
|
|
||
| assert!(Arc::ptr_eq(&handle, &server.autodetect_rtt_handle())); | ||
| assert_eq!(server.autodetect_rtt_handle().load(Ordering::Relaxed), 42); | ||
| } |
Comment on lines
391
to
395
| autodetect: None, | ||
| connection_handler, | ||
| display_suppressed: display_suppressed.unwrap_or_else(|| Arc::new(AtomicBool::new(false))), | ||
| autodetect_rtt: autodetect_rtt.unwrap_or_else(|| Arc::new(AtomicU32::new(u32::MAX))), | ||
| } |
Comment on lines
+105
to
+116
| let handle = Arc::new(AtomicU32::new(42)); | ||
| let server = RdpServer::builder() | ||
| .with_addr(SocketAddr::from((Ipv4Addr::LOCALHOST, 0))) | ||
| .with_no_security() | ||
| .with_no_input() | ||
| .with_no_display() | ||
| .with_autodetect_rtt_handle(Arc::clone(&handle)) | ||
| .build(); | ||
|
|
||
| assert!(Arc::ptr_eq(&handle, &server.autodetect_rtt_handle())); | ||
| assert_eq!(server.autodetect_rtt_handle().load(Ordering::Relaxed), 42); | ||
| } |
Benoît Cortier (CBenoit)
approved these changes
Jun 1, 2026
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
Member
There was a problem hiding this comment.
This is looking good to me, but I have a question for you.
Using an Arc<AtomicU32> to share a state is of course perfectly fine, but don’t you end up polling the value repeatedly? I was wondering if a callback-based API would not be superior.
I don’t have the full consumer context, so maybe a simple Arc<Atomic*> is enough and does not perform poorly at all (e.g.: it’s optimal for a "read on a per-need basis model).
| clippy::too_many_arguments, | ||
| reason = "called via the builder; positional parameters are an internal detail" | ||
| )] | ||
| pub fn new( |
There was a problem hiding this comment.
thought: We need to make this function private
The server measures network RTT in its AutoDetectManager, but after run() takes ownership a backend can no longer call rtt_snapshot(): the measurement is stranded in the running server. Mirror the existing display_suppressed handle so the value can be shared out. Add an Arc<AtomicU32> holding the latest measured RTT in milliseconds (u32::MAX until the first measurement), a with_autodetect_rtt_handle builder injector, and an autodetect_rtt_handle() getter. The AutoDetectRsp handler stores the measured rtt_ms into it. A display backend can then read a fresh, frame-traffic-independent network RTT for flow control without polling the server object. Drop the cfg_attr(egfx) gate on the new() too_many_arguments expect: the added parameter makes the non-egfx parameter count 8 (was 7), so the lint now fires in both feature configs and the expect is satisfied unconditionally.
Greg Lamberson (glamberson)
force-pushed
the
feat/server-autodetect-rtt-handle
branch
from
177e183 to
4f5bd55
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The server measures network RTT in its AutoDetectManager (added in #1177), but once run() takes ownership a backend can no longer call rtt_snapshot() to read it: the measurement is stranded in the running server. This mirrors the existing display_suppressed shared-handle pattern (#1319) so the value can be shared out.
Adds an Arc holding the latest measured RTT in milliseconds (u32::MAX until the first measurement, and while auto-detect is disabled), a with_autodetect_rtt_handle builder injector, and an autodetect_rtt_handle() getter. The AutoDetectRsp handler stores the measured rtt_ms into it. A display backend can then read a fresh, frame-traffic-independent network RTT for flow control (for example to size an encoder's in-flight window) without polling the server object.
This also drops the cfg_attr(egfx) gate on new()'s too_many_arguments expect: the added parameter makes the non-egfx parameter count 8 (it was exactly 7, where the lint was silent), so the lint now fires in both feature configs and the expect is satisfied unconditionally.
Validation
cargo xtask check fmt/lints/tests/typos/locks all pass. New tests in ironrdp-testsuite-core cover the u32::MAX default and that with_autodetect_rtt_handle round-trips the same Arc.
Notes
Additive; mirrors #1319's handle pattern. new() gains a parameter the same way #1319 added display_suppressed (feat, not feat!, per that precedent). Exposes only the generic RTT value; no flow-control policy.