Skip to content

Commit 87db9cb

Browse files
committed
fix: cancel upload probes before recording starts
1 parent 6a52a63 commit 87db9cb

6 files changed

Lines changed: 242 additions & 9 deletions

File tree

apps/desktop/src-tauri/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,4 +186,4 @@ nix = { version = "0.29.0", features = ["fs"] }
186186

187187
[dev-dependencies]
188188
tauri = { workspace = true, features = ["test"] }
189-
tokio = { workspace = true, features = ["test-util"] }
189+
tokio = { workspace = true, features = ["test-util", "net", "io-util"] }

apps/desktop/src-tauri/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,6 +1150,7 @@ impl App {
11501150
}
11511151

11521152
self.recording_state = RecordingState::Pending { mode, target };
1153+
upload_health::cancel_probe_for_recording(&self.handle);
11531154
CurrentRecordingChanged.emit(&self.handle).ok();
11541155

11551156
Ok(())

apps/desktop/src-tauri/src/recording.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1712,6 +1712,8 @@ async fn start_recording_prepared(
17121712
}
17131713
}
17141714

1715+
crate::upload_health::wait_for_probe_to_stop(&app).await;
1716+
17151717
if cfg!(target_os = "linux") && inputs.mode == RecordingMode::Instant {
17161718
drop(_input_operation.take());
17171719
}

apps/desktop/src-tauri/src/upload_health.rs

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,13 @@ use crate::{
1111
web_api::{AuthedApiError, ManagerExt},
1212
};
1313

14+
mod lifecycle;
1415
mod timing;
1516

16-
use timing::{measure_warm_probe_rtt, upload_elapsed_after_rtt, upload_mbps_for_bytes};
17+
use lifecycle::ProbeControl;
18+
use timing::{
19+
connection_will_close, measure_warm_probe_rtt, upload_elapsed_after_rtt, upload_mbps_for_bytes,
20+
};
1721

1822
const PROBE_BYTES: usize = 256 * 1024;
1923
const HEALTH_FRESH_FOR: Duration = Duration::from_secs(10 * 60);
@@ -90,7 +94,7 @@ impl UploadHealthSnapshot {
9094
#[derive(Default)]
9195
pub struct UploadHealthCache {
9296
snapshot: Mutex<UploadHealthSnapshot>,
93-
probe: Mutex<()>,
97+
probe: ProbeControl,
9498
}
9599

96100
impl UploadHealthCache {
@@ -159,7 +163,22 @@ async fn measure_probe_rtt(app: &AppHandle) -> Option<Duration> {
159163
.await;
160164

161165
match response {
162-
Ok(response) if response.status().is_success() => Some(started.elapsed()),
166+
Ok(response) if response.status().is_success() => {
167+
let closes_connection = response.version() == reqwest::Version::HTTP_10
168+
|| response
169+
.headers()
170+
.get_all(reqwest::header::CONNECTION)
171+
.iter()
172+
.any(|value| match value.to_str() {
173+
Ok(value) => connection_will_close(value),
174+
Err(_) => true,
175+
});
176+
if closes_connection {
177+
None
178+
} else {
179+
Some(started.elapsed())
180+
}
181+
}
163182
Ok(response) => {
164183
let status = response.status();
165184
debug!(%status, "Upload health RTT probe returned a non-success status");
@@ -298,18 +317,36 @@ pub async fn refresh_upload_health_status(
298317
app_state: MutableState<'_, App>,
299318
cache: State<'_, UploadHealthCache>,
300319
) -> Result<UploadHealthStatus, String> {
301-
if app_state.read().await.is_recording_active_or_pending() {
320+
let state = app_state.read().await;
321+
if state.is_recording_active_or_pending() {
322+
drop(state);
302323
return Ok(cache.status().await);
303324
}
304325

305-
let Ok(_probe_guard) = cache.probe.try_lock() else {
326+
let Some(mut probe) = cache.probe.try_start() else {
327+
drop(state);
306328
return Ok(cache.status().await);
307329
};
330+
drop(state);
308331

309-
let snapshot = run_probe(&app).await;
332+
let Some(snapshot) = probe.run(run_probe(&app)).await else {
333+
return Ok(cache.status().await);
334+
};
310335
Ok(cache.update(snapshot).await)
311336
}
312337

338+
pub fn cancel_probe_for_recording(app: &AppHandle) {
339+
if let Some(cache) = app.try_state::<UploadHealthCache>() {
340+
cache.probe.cancel();
341+
}
342+
}
343+
344+
pub async fn wait_for_probe_to_stop(app: &AppHandle) {
345+
if let Some(cache) = app.try_state::<UploadHealthCache>() {
346+
cache.probe.cancel_and_wait().await;
347+
}
348+
}
349+
313350
pub async fn cached_instant_resolution_cap(app: &AppHandle) -> Option<u32> {
314351
let cache = app.try_state::<UploadHealthCache>()?;
315352
cache.fresh_instant_resolution_cap().await
@@ -338,7 +375,7 @@ mod tests {
338375
recorded_at: Some(Instant::now() - HEALTH_FRESH_FOR - Duration::from_secs(1)),
339376
message: "old".to_string(),
340377
}),
341-
probe: Mutex::new(()),
378+
probe: ProbeControl::default(),
342379
};
343380

344381
assert_eq!(cache.fresh_instant_resolution_cap().await, None);
@@ -355,7 +392,7 @@ mod tests {
355392
recorded_at: Some(Instant::now()),
356393
message: "slow".to_string(),
357394
}),
358-
probe: Mutex::new(()),
395+
probe: ProbeControl::default(),
359396
};
360397

361398
assert_eq!(cache.fresh_instant_resolution_cap().await, Some(1280));
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
use std::{future::Future, pin::Pin};
2+
3+
use tokio::sync::{Mutex, MutexGuard, Notify, futures::Notified};
4+
5+
#[derive(Default)]
6+
pub(super) struct ProbeControl {
7+
running: Mutex<()>,
8+
cancelled: Notify,
9+
}
10+
11+
impl ProbeControl {
12+
pub(super) fn try_start(&self) -> Option<ActiveProbe<'_>> {
13+
let running = self.running.try_lock().ok()?;
14+
let mut cancelled = Box::pin(self.cancelled.notified());
15+
cancelled.as_mut().enable();
16+
Some(ActiveProbe {
17+
_running: running,
18+
cancelled,
19+
})
20+
}
21+
22+
pub(super) fn cancel(&self) {
23+
self.cancelled.notify_waiters();
24+
}
25+
26+
pub(super) async fn cancel_and_wait(&self) {
27+
self.cancel();
28+
drop(self.running.lock().await);
29+
}
30+
}
31+
32+
pub(super) struct ActiveProbe<'a> {
33+
_running: MutexGuard<'a, ()>,
34+
cancelled: Pin<Box<Notified<'a>>>,
35+
}
36+
37+
impl ActiveProbe<'_> {
38+
pub(super) async fn run<Probe: Future>(&mut self, probe: Probe) -> Option<Probe::Output> {
39+
tokio::select! {
40+
biased;
41+
() = &mut self.cancelled => None,
42+
result = probe => Some(result),
43+
}
44+
}
45+
}
46+
47+
#[cfg(test)]
48+
mod tests {
49+
use std::{
50+
future::{pending, ready},
51+
sync::{
52+
Arc,
53+
atomic::{AtomicBool, Ordering},
54+
},
55+
};
56+
57+
use tokio::{io::AsyncReadExt, net::TcpListener, sync::oneshot};
58+
59+
use super::*;
60+
61+
#[tokio::test]
62+
async fn cancellation_before_first_poll_does_not_start_the_request() {
63+
let control = ProbeControl::default();
64+
let mut active = control.try_start().expect("probe should start");
65+
control.cancel();
66+
67+
let result = active
68+
.run(async { panic!("a cancelled probe must not send an HTTP request") })
69+
.await;
70+
71+
assert_eq!(result, None);
72+
}
73+
74+
#[tokio::test]
75+
async fn only_one_probe_can_run_at_a_time() {
76+
let control = ProbeControl::default();
77+
let mut active = control.try_start().expect("probe should start");
78+
assert!(control.try_start().is_none());
79+
assert_eq!(active.run(ready(42)).await, Some(42));
80+
assert!(control.try_start().is_none());
81+
drop(active);
82+
assert!(control.try_start().is_some());
83+
}
84+
85+
struct DropFlag(Arc<AtomicBool>);
86+
87+
impl Drop for DropFlag {
88+
fn drop(&mut self) {
89+
self.0.store(true, Ordering::SeqCst);
90+
}
91+
}
92+
93+
#[tokio::test]
94+
async fn recording_waits_until_in_flight_request_is_dropped() {
95+
let control = Arc::new(ProbeControl::default());
96+
let request_dropped = Arc::new(AtomicBool::new(false));
97+
let (started_tx, started_rx) = oneshot::channel();
98+
let probe_control = Arc::clone(&control);
99+
let dropped = Arc::clone(&request_dropped);
100+
let task = tokio::spawn(async move {
101+
let mut active = probe_control.try_start().expect("probe should start");
102+
active
103+
.run(async move {
104+
let _drop_flag = DropFlag(dropped);
105+
started_tx.send(()).expect("receiver should be waiting");
106+
pending::<()>().await;
107+
})
108+
.await
109+
});
110+
111+
started_rx
112+
.await
113+
.expect("probe should enter the upload POST");
114+
control.cancel_and_wait().await;
115+
116+
assert!(request_dropped.load(Ordering::SeqCst));
117+
assert_eq!(task.await.expect("probe task should not panic"), None);
118+
assert!(control.try_start().is_some());
119+
}
120+
121+
#[tokio::test]
122+
async fn cancels_a_real_http_request_waiting_for_its_response() {
123+
let listener = TcpListener::bind(("127.0.0.1", 0))
124+
.await
125+
.expect("test listener should bind");
126+
let url = format!("http://{}/upload-health", listener.local_addr().unwrap());
127+
let (request_tx, request_rx) = oneshot::channel();
128+
let (stop_tx, stop_rx) = oneshot::channel();
129+
let server = tokio::spawn(async move {
130+
let (mut socket, _) = listener.accept().await.unwrap();
131+
let mut method = [0; 5];
132+
socket.read_exact(&mut method).await.unwrap();
133+
assert_eq!(&method, b"POST ");
134+
request_tx.send(()).unwrap();
135+
stop_rx.await.unwrap();
136+
});
137+
let control = Arc::new(ProbeControl::default());
138+
let probe_control = Arc::clone(&control);
139+
let request = tokio::spawn(async move {
140+
let mut active = probe_control.try_start().expect("probe should start");
141+
active
142+
.run(async {
143+
reqwest::Client::builder()
144+
.no_proxy()
145+
.build()
146+
.unwrap()
147+
.post(url)
148+
.body(vec![0; 256 * 1024])
149+
.send()
150+
.await
151+
})
152+
.await
153+
});
154+
155+
tokio::time::timeout(std::time::Duration::from_secs(5), async {
156+
request_rx.await.unwrap();
157+
control.cancel_and_wait().await;
158+
assert!(request.await.unwrap().is_none());
159+
stop_tx.send(()).unwrap();
160+
server.await.unwrap();
161+
})
162+
.await
163+
.expect("cancellation should not wait for the HTTP response");
164+
}
165+
166+
#[tokio::test]
167+
async fn previous_cancellation_does_not_cancel_a_later_probe() {
168+
let control = ProbeControl::default();
169+
let mut active = control.try_start().expect("probe should start");
170+
control.cancel();
171+
assert_eq!(active.run(ready(1)).await, None);
172+
drop(active);
173+
174+
control.cancel_and_wait().await;
175+
let mut next = control.try_start().expect("next probe should start");
176+
assert_eq!(next.run(ready(2)).await, Some(2));
177+
}
178+
}

apps/desktop/src-tauri/src/upload_health/timing.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
use std::{future::Future, time::Duration};
22

3+
pub(super) fn connection_will_close(value: &str) -> bool {
4+
value
5+
.split(',')
6+
.any(|token| token.trim().eq_ignore_ascii_case("close"))
7+
}
8+
39
pub(super) async fn measure_warm_probe_rtt<Probe, ProbeFuture>(
410
budget: Duration,
511
mut probe: Probe,
@@ -44,6 +50,15 @@ mod tests {
4450

4551
use super::*;
4652

53+
#[test]
54+
fn detects_close_among_connection_tokens() {
55+
assert!(connection_will_close("close"));
56+
assert!(connection_will_close("keep-alive, CLOSE"));
57+
assert!(connection_will_close(" Close , upgrade"));
58+
assert!(!connection_will_close("keep-alive"));
59+
assert!(!connection_will_close(""));
60+
}
61+
4762
#[tokio::test]
4863
async fn cold_connection_time_does_not_inflate_upload_speed() {
4964
let mut samples = [

0 commit comments

Comments
 (0)