Skip to content
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

Fix broken grateful shutdown for tracker API #589

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bin/http_health_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ async fn main() {
let args: Vec<String> = env::args().collect();
if args.len() != 2 {
eprintln!("Usage: cargo run --bin http_health_check <HEALTH_URL>");
eprintln!("Example: cargo run --bin http_health_check http://127.0.0.1:1212/api/health_check");
eprintln!("Example: cargo run --bin http_health_check http://127.0.0.1:1313/health_check");
std::process::exit(1);
}

Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/jobs/health_check_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@

// Wait until the API server job is running
match rx_start.await {
Ok(msg) => info!("Torrust Health Check API server started on socket: {}", msg.address),
Ok(msg) => info!("Torrust Health Check API server started on: http://{}", msg.address),

Check warning on line 58 in src/bootstrap/jobs/health_check_api.rs

View check run for this annotation

Codecov / codecov/patch

src/bootstrap/jobs/health_check_api.rs#L58

Added line #L58 was not covered by tests
Err(e) => panic!("the Health Check API server was dropped: {e}"),
}

Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/jobs/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub struct Started {

pub async fn make_rust_tls(enabled: bool, cert: &Option<String>, key: &Option<String>) -> Option<Result<RustlsConfig, Error>> {
if !enabled {
info!("tls not enabled");
info!("TLS not enabled");
return None;
}

Expand Down
1 change: 1 addition & 0 deletions src/bootstrap/jobs/tracker_apis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
.expect("it should be able to start to the tracker api");

tokio::spawn(async move {
assert!(!server.state.halt_task.is_closed(), "Halt channel should be open");

Check warning on line 83 in src/bootstrap/jobs/tracker_apis.rs

View check run for this annotation

Codecov / codecov/patch

src/bootstrap/jobs/tracker_apis.rs#L83

Added line #L83 was not covered by tests
server.state.task.await.expect("failed to close service");
})
}
Expand Down
33 changes: 23 additions & 10 deletions src/servers/apis/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use axum_server::Handle;
use derive_more::Constructor;
use futures::future::BoxFuture;
use log::{error, info};
use tokio::sync::oneshot::{Receiver, Sender};

use super::routes::router;
Expand Down Expand Up @@ -101,13 +102,22 @@
launcher
});

Ok(ApiServer {
state: Running {
binding: rx_start.await.expect("unable to start service").address,
halt_task: tx_halt,
task,
let api_server = match rx_start.await {
Ok(started) => ApiServer {
state: Running {
binding: started.address,
halt_task: tx_halt,
task,
},
},
})
Err(err) => {
let msg = format!("Unable to start API server: {err}");
error!("{}", msg);
panic!("{}", msg);

Check warning on line 116 in src/servers/apis/server.rs

View check run for this annotation

Codecov / codecov/patch

src/servers/apis/server.rs#L114-L116

Added lines #L114 - L116 were not covered by tests
}
};

Ok(api_server)
}
}

Expand Down Expand Up @@ -159,29 +169,32 @@
tokio::task::spawn(graceful_shutdown(
handle.clone(),
rx_halt,
format!("shutting down http server on socket address: {address}"),
format!("Shutting down tracker API server on socket address: {address}"),
));

let tls = self.tls.clone();
let protocol = if tls.is_some() { "https" } else { "http" };

let running = Box::pin(async {
match tls {
Some(tls) => axum_server::from_tcp_rustls(socket, tls)
.handle(handle)
.serve(router.into_make_service_with_connect_info::<std::net::SocketAddr>())
.await
.expect("Axum server crashed."),
.expect("Axum server for tracker API crashed."),

Check warning on line 184 in src/servers/apis/server.rs

View check run for this annotation

Codecov / codecov/patch

src/servers/apis/server.rs#L184

Added line #L184 was not covered by tests
None => axum_server::from_tcp(socket)
.handle(handle)
.serve(router.into_make_service_with_connect_info::<std::net::SocketAddr>())
.await
.expect("Axum server crashed."),
.expect("Axum server for tracker API crashed."),
}
});

info!(target: "API", "API server started on {protocol}://{}", address);

tx_start
.send(Started { address })
.expect("the HTTP(s) Tracker service should not be dropped");
.expect("the HTTP(s) Tracker API service should not be dropped");

running
}
Expand Down
2 changes: 1 addition & 1 deletion src/servers/http/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl Launcher {
tokio::task::spawn(graceful_shutdown(
handle.clone(),
rx_halt,
format!("shutting down http server on socket address: {address}"),
format!("Shutting down http server on socket address: {address}"),
));

let tls = self.tls.clone();
Expand Down
13 changes: 9 additions & 4 deletions src/servers/signals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,16 @@
///
/// Will panic if the `stop_receiver` resolves with an error.
pub async fn shutdown_signal(rx_halt: tokio::sync::oneshot::Receiver<Halted>) {
let halt = async { rx_halt.await.expect("Failed to install stop signal.") };
let halt = async {
match rx_halt.await {
Ok(signal) => signal,
Err(err) => panic!("Failed to install stop signal: {err}"),

Check warning on line 52 in src/servers/signals.rs

View check run for this annotation

Codecov / codecov/patch

src/servers/signals.rs#L52

Added line #L52 was not covered by tests
}
};

tokio::select! {
_ = halt => {},
() = global_shutdown_signal() => {}
signal = halt => { info!("Halt signal processed: {}", signal) },
() = global_shutdown_signal() => { info!("Global shutdown signal processed") }
}
}

Expand All @@ -64,7 +69,7 @@
pub async fn graceful_shutdown(handle: axum_server::Handle, rx_halt: tokio::sync::oneshot::Receiver<Halted>, message: String) {
shutdown_signal_with_message(rx_halt, message).await;

info!("sending graceful shutdown signal");
info!("Sending graceful shutdown signal");
handle.graceful_shutdown(Some(Duration::from_secs(90)));

println!("!! shuting down in 90 seconds !!");
Expand Down