From cf613b8a1f66cd674ac39a05cb956ce89cc12052 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 9 Jan 2024 16:01:16 +0000 Subject: [PATCH 1/4] fix: [#588] broken grateful shutdown for tracker API The internal halt channel was nor working becuase the sender was being droped just after starting the server. That also made the `shutdown_signal` fail. ```rust pub async fn shutdown_signal(rx_halt: tokio::sync::oneshot::Receiver) { let halt = async { match rx_halt.await { Ok(signal) => signal, Err(err) => panic!("Failed to install stop signal: {err}"), } }; tokio::select! { signal = halt => { info!("Halt signal processed: {}", signal) }, () = global_shutdown_signal() => { info!("Global shutdown signal processed") } } } ``` Since the signal branch in the `tokio::select!` was finishing the global_shutdown_signal did not work either. So you had to kill the process manually to stop the tracker. It seems Rust droped partially the `Running::halt_taks` attribute and that closed the channel. --- src/bootstrap/jobs/tracker_apis.rs | 1 + src/servers/apis/server.rs | 23 +++++++++++++++++------ src/servers/signals.rs | 11 ++++++++--- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/bootstrap/jobs/tracker_apis.rs b/src/bootstrap/jobs/tracker_apis.rs index f454b017f..e50a83651 100644 --- a/src/bootstrap/jobs/tracker_apis.rs +++ b/src/bootstrap/jobs/tracker_apis.rs @@ -80,6 +80,7 @@ async fn start_v1(socket: SocketAddr, tls: Option, tracker: Arc { launcher }); - Ok(ApiServer { - state: Running { - binding: rx_start.await.expect("unable to start service").address, - halt_task: tx_halt, - task, + //let address = rx_start.await.expect("unable to start service").address; + 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); + } + }; + + Ok(api_server) } } diff --git a/src/servers/signals.rs b/src/servers/signals.rs index cb0675d65..091982ae3 100644 --- a/src/servers/signals.rs +++ b/src/servers/signals.rs @@ -46,11 +46,16 @@ pub async fn global_shutdown_signal() { /// /// Will panic if the `stop_receiver` resolves with an error. pub async fn shutdown_signal(rx_halt: tokio::sync::oneshot::Receiver) { - 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}"), + } + }; tokio::select! { - _ = halt => {}, - () = global_shutdown_signal() => {} + signal = halt => { info!("Halt signal processed: {}", signal) }, + () = global_shutdown_signal() => { info!("Global shutdown signal processed") } } } From 53613ec31eec23f569d3895c90f77ae5105f083d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 9 Jan 2024 16:19:15 +0000 Subject: [PATCH 2/4] feat: start log lines with capital --- src/bootstrap/jobs/mod.rs | 2 +- src/servers/apis/server.rs | 2 +- src/servers/http/server.rs | 2 +- src/servers/signals.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/jobs/mod.rs b/src/bootstrap/jobs/mod.rs index 3a9936882..2c12eb40e 100644 --- a/src/bootstrap/jobs/mod.rs +++ b/src/bootstrap/jobs/mod.rs @@ -22,7 +22,7 @@ pub struct Started { pub async fn make_rust_tls(enabled: bool, cert: &Option, key: &Option) -> Option> { if !enabled { - info!("tls not enabled"); + info!("TLS not enabled"); return None; } diff --git a/src/servers/apis/server.rs b/src/servers/apis/server.rs index f9507b0fb..b885bf348 100644 --- a/src/servers/apis/server.rs +++ b/src/servers/apis/server.rs @@ -170,7 +170,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(); diff --git a/src/servers/http/server.rs b/src/servers/http/server.rs index aee2d0ac0..c3411ac06 100644 --- a/src/servers/http/server.rs +++ b/src/servers/http/server.rs @@ -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(); diff --git a/src/servers/signals.rs b/src/servers/signals.rs index 091982ae3..42fd868e8 100644 --- a/src/servers/signals.rs +++ b/src/servers/signals.rs @@ -69,7 +69,7 @@ pub async fn shutdown_signal_with_message(rx_halt: tokio::sync::oneshot::Receive pub async fn graceful_shutdown(handle: axum_server::Handle, rx_halt: tokio::sync::oneshot::Receiver, 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 !!"); From ac18605ee563ee4f835803012cbbc2486bc97b13 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 9 Jan 2024 16:30:30 +0000 Subject: [PATCH 3/4] feat: improve log when starting the API Added the URL where the API is running. ``` 2024-01-09T16:29:46.911284166+00:00 [API][INFO] API server started on http://127.0.0.1:1212 ``` Some hosting services parse the application log output to discover services. For example, GitHub Codespaces can use that into to automatically do port forwarding. Besides, it's also useful for development when you are using random ports and to see what services are you running. --- src/servers/apis/server.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/servers/apis/server.rs b/src/servers/apis/server.rs index b885bf348..5df1d76fd 100644 --- a/src/servers/apis/server.rs +++ b/src/servers/apis/server.rs @@ -30,7 +30,7 @@ use axum_server::tls_rustls::RustlsConfig; use axum_server::Handle; use derive_more::Constructor; use futures::future::BoxFuture; -use log::error; +use log::{error, info}; use tokio::sync::oneshot::{Receiver, Sender}; use super::routes::router; @@ -102,7 +102,6 @@ impl ApiServer { launcher }); - //let address = rx_start.await.expect("unable to start service").address; let api_server = match rx_start.await { Ok(started) => ApiServer { state: Running { @@ -112,7 +111,7 @@ impl ApiServer { }, }, Err(err) => { - let msg = format!("unable to start API server: {err}"); + let msg = format!("Unable to start API server: {err}"); error!("{}", msg); panic!("{}", msg); } @@ -170,10 +169,11 @@ impl Launcher { 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 { @@ -181,18 +181,20 @@ impl Launcher { .handle(handle) .serve(router.into_make_service_with_connect_info::()) .await - .expect("Axum server crashed."), + .expect("Axum server for tracker API crashed."), None => axum_server::from_tcp(socket) .handle(handle) .serve(router.into_make_service_with_connect_info::()) .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 } From 452b4a0cd665e3e27d0e2440ea06e30a707eac00 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 9 Jan 2024 16:55:23 +0000 Subject: [PATCH 4/4] feat: improve http_health_check output --- src/bin/http_health_check.rs | 2 +- src/bootstrap/jobs/health_check_api.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bin/http_health_check.rs b/src/bin/http_health_check.rs index d3f1767cb..d66d334df 100644 --- a/src/bin/http_health_check.rs +++ b/src/bin/http_health_check.rs @@ -11,7 +11,7 @@ async fn main() { let args: Vec = env::args().collect(); if args.len() != 2 { eprintln!("Usage: cargo run --bin http_health_check "); - 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); } diff --git a/src/bootstrap/jobs/health_check_api.rs b/src/bootstrap/jobs/health_check_api.rs index 83eb77f6b..a49f612e8 100644 --- a/src/bootstrap/jobs/health_check_api.rs +++ b/src/bootstrap/jobs/health_check_api.rs @@ -55,7 +55,7 @@ pub async fn start_job(config: Arc) -> JoinHandle<()> { // 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), Err(e) => panic!("the Health Check API server was dropped: {e}"), }