Repository navigation
atomesh: make the fan-out request timeout a CLI option (--worker-request-timeout-secs) #478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,14 @@ | |
| //! | ||
| //! Provides worker lifecycle operations and fan-out request utilities. | ||
|
|
||
| use std::{collections::HashMap, sync::Arc, time::Duration}; | ||
| use std::{ | ||
| collections::HashMap, | ||
| sync::{ | ||
| atomic::{AtomicU64, Ordering}, | ||
| Arc, | ||
| }, | ||
| time::Duration, | ||
| }; | ||
|
|
||
| use futures::{ | ||
| future, | ||
|
|
@@ -21,7 +28,18 @@ use crate::{ | |
| protocols::worker_spec::{FlushCacheResult, WorkerLoadInfo, WorkerLoadsResult}, | ||
| }; | ||
|
|
||
| const REQUEST_TIMEOUT: Duration = Duration::from_secs(5); | ||
| pub const DEFAULT_WORKER_REQUEST_TIMEOUT_SECS: u64 = 5; | ||
|
|
||
| /// Timeout for the `/flush_cache` and `/get_load` requests below; set from | ||
| /// `--worker-request-timeout-secs` when the router config is built. | ||
| /// Process-wide: the last router config built in a process wins. | ||
| pub static WORKER_REQUEST_TIMEOUT_SECS: AtomicU64 = | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reservation, not blocking: the process-wide static is acceptable and the leaner choice. Suggestion: one comment line on the static naming the ceiling, e.g. "process-wide: the last router config built wins; move into RouterConfig if a process ever builds two." What the alternative costs, and the static's costs, at this headMoving the value into
|
||
| AtomicU64::new(DEFAULT_WORKER_REQUEST_TIMEOUT_SECS); | ||
|
|
||
| fn request_timeout() -> Duration { | ||
| Duration::from_secs(WORKER_REQUEST_TIMEOUT_SECS.load(Ordering::Relaxed)) | ||
| } | ||
|
|
||
| const MAX_CONCURRENT: usize = 32; | ||
|
|
||
| /// Result of a fan-out request to a single worker | ||
|
|
@@ -47,7 +65,7 @@ async fn fan_out( | |
| let method = method.clone(); | ||
|
|
||
| async move { | ||
| let mut req = client.request(method, &full_url).timeout(REQUEST_TIMEOUT); | ||
| let mut req = client.request(method, &full_url).timeout(request_timeout()); | ||
| if let Some(key) = api_key { | ||
| req = req.bearer_auth(key); | ||
| } | ||
|
|
@@ -194,7 +212,7 @@ impl WorkerManager { | |
| api_key: Option<&str>, | ||
| ) -> isize { | ||
| let load_url = format!("{}/get_load", url); | ||
| let mut req = client.get(&load_url).timeout(REQUEST_TIMEOUT); | ||
| let mut req = client.get(&load_url).timeout(request_timeout()); | ||
| if let Some(key) = api_key { | ||
| req = req.bearer_auth(key); | ||
| } | ||
|
|
@@ -340,3 +358,72 @@ impl Drop for LoadMonitor { | |
| } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::time::Instant; | ||
|
|
||
| use clap::Parser; | ||
|
|
||
| use super::*; | ||
| use crate::{cliargs::CliArgs, core::BasicWorkerBuilder}; | ||
|
|
||
| #[tokio::test] | ||
| async fn worker_request_timeout_option_outlasts_a_40s_worker() { | ||
| let slow = Duration::from_secs(40); | ||
| let app = axum::Router::new() | ||
| .route( | ||
| "/get_load", | ||
| axum::routing::get(move || async move { | ||
| tokio::time::sleep(slow).await; | ||
| axum::Json(serde_json::json!([{ "num_tokens": 7 }])) | ||
| }), | ||
| ) | ||
| .route( | ||
| "/flush_cache", | ||
| axum::routing::post(move || tokio::time::sleep(slow)), | ||
| ); | ||
| let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); | ||
| let url = format!("http://{}", listener.local_addr().unwrap()); | ||
| tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); | ||
|
|
||
| let registry = WorkerRegistry::new(); | ||
| registry.register(Arc::new(BasicWorkerBuilder::new(&url).build())); | ||
| let client = reqwest::Client::new(); | ||
|
|
||
| for (flags, answered) in [ | ||
| (&[][..], false), | ||
| (&["--worker-request-timeout-secs", "60"][..], true), | ||
| ] { | ||
| let args = CliArgs::parse_from(["atomesh"].iter().chain(flags)); | ||
| args.to_router_config(vec![]).unwrap(); | ||
| let start = Instant::now(); | ||
| let (loads, flush) = tokio::join!( | ||
| WorkerManager::get_all_worker_loads(®istry, &client), | ||
| WorkerManager::flush_cache_all(®istry, &client), | ||
| ); | ||
| let elapsed = start.elapsed(); | ||
| eprintln!( | ||
| "flags={flags:?} load={} flushed={} elapsed={elapsed:?}", | ||
| loads.loads[0].load, | ||
| flush.successful.len() | ||
| ); | ||
| assert_eq!(loads.loads[0].load, if answered { 7 } else { -1 }); | ||
| assert_eq!(flush.successful.len(), answered as usize); | ||
| assert_eq!(elapsed >= slow, answered); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Required finding 2: "defaults unchanged" is pinned only for a default of 40 s or more, because the no-flag leg's only time bound is The mutant
|
||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn worker_request_timeout_defaults_to_5s_and_refuses_zero() { | ||
| let parse = |flags: &[&str]| CliArgs::try_parse_from(["atomesh"].iter().chain(flags)); | ||
| assert_eq!(parse(&[]).unwrap().worker_request_timeout_secs, 5); | ||
| assert_eq!( | ||
| parse(&["--worker-request-timeout-secs", "1"]) | ||
| .unwrap() | ||
| .worker_request_timeout_secs, | ||
| 1 | ||
| ); | ||
| assert!(parse(&["--worker-request-timeout-secs", "0"]).is_err()); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reservation, not blocking, and a landing-order note. This row keeps category
C1while its new, correctwhysays it bounds no request; changing the category changes the audit README counts, outside this PR, so it is recorded on #477. This file conflicts with #476: whichever lands second must keep this PR'sfile/line/anchor/whyon all four rows and add #476'smechanismfields.Detail
Category: the audit README defines C1 as "a bound that exists to declare something broken: raise it, or switch it off", and counts the row in "five router and server bounds". The
whyis correct: the client has one use (http_health_check), and reqwest 0.12.28RequestConfig::fetchreturns the request's timeout and falls back to the client default only when the request has none, so it replaces the default rather than taking the minimum.Conflict:
git merge-tree --write-tree 67117f738 b28823d9dreportsCONFLICT (content)here. #476 (delivers #454) still hasworker_manager.rs:24 const REQUEST_TIMEOUTandcliargs.rs:423/:398on these rows, and gives this rowmechanism: K8. #476 carriesneed human, so this PR will most likely land first, and #476's base-update merge must do the keeping. If it keeps #476's line numbers,test_anchor_lines_are_still_where_they_sayfails with exactly the three messages recorded in this PR's body. K8 on this row needs a second look for the same reason as the category.