From a0bb909f4ee50a9cc84096f65b53926ccdf6fdc9 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Tue, 28 Jul 2026 10:21:51 -0300 Subject: [PATCH] drm: make the outer handshake budget dominate the inner one Two findings from the review bot on our own fork, both worth taking. The caller waited HANDSHAKE_TIMEOUT_MS + 500 for the receive thread to hand back the display list, but that thread is allowed to spend more than that: the connect budget, and then recv_msg_timeout2 applies its argument twice in the worst case, once waiting for the first byte and once for the body. So on a slow connect the outer timer fired first and abandoned a handshake that was still inside its own budget. The wait is now derived from those parts rather than written as a constant, so changing either one cannot silently invert the relationship again, and the two connect sites use the named constant instead of a literal. The cursor cache insert shadowed hcursor under a cfg, so the same line meant the requested id in one build and the served id in the other. It is a separate name now, with the reason on it. Not taken, and why: the bot also suggested making DrmCursorData carry width and height as u32 to match the wire. They are i32 because that is what they feed, protobuf CursorData declares both as int32 and platform/linux.rs assigns them straight across. One cast has to exist somewhere, and it belongs at the boundary where the values are already being validated, not at the consumer. 101 tests pass, both configs build. --- src/server/drm_capturer.rs | 20 ++++++++++++++------ src/server/input_service.rs | 16 ++++++++++++---- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 161ceb257..d3d0694ef 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -33,9 +33,17 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Condvar, Mutex}; use std::time::{Duration, Instant}; -// Upper bound on how long `new()` waits for the service to answer with the display list before -// giving up and letting the caller fall back. +// Upper bound on how long the receive thread waits for the service to answer with the display list. const HANDSHAKE_TIMEOUT_MS: u64 = 3000; +// How long that thread may spend connecting to `_drm` before the handshake starts. +const DRM_CONNECT_TIMEOUT_MS: u64 = 1000; +/// How long a caller waits for the receive thread to hand back the display list. It must DOMINATE +/// what that thread is allowed to spend, or the outer timer fires first and abandons a handshake +/// that was still inside its own budget: the thread spends up to the connect timeout, then +/// `recv_msg_timeout2` applies HANDSHAKE_TIMEOUT_MS TWICE in the worst case (once waiting for the +/// first byte, once for the body). Derived from those parts rather than written as a constant, so a +/// change to either one cannot silently invert the relationship again. +const HANDSHAKE_WAIT_MS: u64 = DRM_CONNECT_TIMEOUT_MS + HANDSHAKE_TIMEOUT_MS * 2 + 500; struct FrameSlot { // (width, height, pixel format, packed pixels) of the newest frame not yet consumed by @@ -271,7 +279,7 @@ impl IpcDrmCapturer { let stop = stop.clone(); std::thread::spawn(move || recv_thread(display, shared, stop, tx)); } - let displays = match rx.recv_timeout(Duration::from_millis(HANDSHAKE_TIMEOUT_MS + 500)) { + let displays = match rx.recv_timeout(Duration::from_millis(HANDSHAKE_WAIT_MS)) { Ok(res) => res?, Err(_) => { // The recv thread still has its own connect/handshake budget. If we just returned, @@ -429,7 +437,7 @@ async fn recv_thread( // remove_drm_cursor); a rebuilt stream for the same display index gets a newer epoch. let cursor_epoch = next_cursor_epoch(); // Handshake: connect, receive the display list, request the display. - let mut conn = match connect_drm(1000).await { + let mut conn = match connect_drm(DRM_CONNECT_TIMEOUT_MS).await { Ok(c) => c, Err(err) => { let _ = tx.send(Err(err)); @@ -908,13 +916,13 @@ fn query_displays() -> ResultType> { std::thread::spawn(move || { let _ = tx.send(query_displays_async()); }); - rx.recv_timeout(Duration::from_millis(HANDSHAKE_TIMEOUT_MS + 1000)) + rx.recv_timeout(Duration::from_millis(HANDSHAKE_WAIT_MS)) .map_err(|_| anyhow!("drm display query timed out"))? } #[tokio::main(flavor = "current_thread")] async fn query_displays_async() -> ResultType> { - let mut conn = connect_drm(1000).await?; + let mut conn = connect_drm(DRM_CONNECT_TIMEOUT_MS).await?; match conn.recv_msg_timeout2(HANDSHAKE_TIMEOUT_MS).await { Some(Ok((Data::DrmDisplayList(v), _fd))) => Ok(v), Some(Ok((other, _fd))) => Err(anyhow!("expected DrmDisplayList, got {:?}", other)), diff --git a/src/server/input_service.rs b/src/server/input_service.rs index 59a3555ac..8b0020ad2 100644 --- a/src/server/input_service.rs +++ b/src/server/input_service.rs @@ -408,18 +408,26 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()> msg = cached.clone(); } else { let mut data = crate::get_cursor_data(hcursor)?; + // File the shape under the id ACTUALLY served, not the one requested. Deliberately a + // NEW name rather than shadowing `hcursor`: the insert below reads as the requested + // id everywhere else in this function, and a cfg-gated shadow would make the two + // builds disagree about what that line means. #[cfg(all(target_os = "linux", feature = "drm"))] - let hcursor = data.id; + let served_id = data.id; #[cfg(all(target_os = "linux", feature = "drm"))] { - drm_served_id = hcursor; + drm_served_id = served_id; } + #[cfg(all(target_os = "linux", feature = "drm"))] + let cache_key = served_id; + #[cfg(not(all(target_os = "linux", feature = "drm")))] + let cache_key = hcursor; data.colors = hbb_common::compress::compress(&data.colors[..]).into(); let mut tmp = Message::new(); tmp.set_cursor_data(data); msg = Arc::new(tmp); - state.cached_cursor_data.insert(hcursor, msg.clone()); - super::log::trace!("Cursor data updated, hcursor: {}", hcursor); + state.cached_cursor_data.insert(cache_key, msg.clone()); + super::log::trace!("Cursor data updated, hcursor: {}", cache_key); } #[cfg(not(all(target_os = "linux", feature = "drm")))] {