Skip to content
Draft
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bin/router/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,4 @@ mimalloc = { version = "0.1.47", features = ["override"] }
moka = { version = "0.12.10", features = ["future"] }
ulid = "1.2.1"
ntex = { version = "2", features = ["tokio"] }
tokio-util = "0.7.16"
51 changes: 51 additions & 0 deletions bin/router/src/background_tasks/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
use async_trait::async_trait;
use futures::future::join_all;
use std::sync::Arc;
use tokio::task::JoinHandle;
use tokio_util::sync::CancellationToken;
use tracing::{debug, info};

#[async_trait]
pub trait BackgroundTask: Send + Sync {

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / linux_amd64

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / macos_arm64

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / linux_arm64

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / macos_amd64

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / cargo build

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / cargo test

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / benchmark / router

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / federation-audit

trait `BackgroundTask` is never used

Check failure on line 9 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / graphql-over-http

trait `BackgroundTask` is never used
fn id(&self) -> &str;
async fn run(&self, token: CancellationToken);
}

pub struct BackgroundTasksManager {
cancellation_token: CancellationToken,
task_handles: Vec<JoinHandle<()>>,
}

impl BackgroundTasksManager {
pub fn new() -> Self {
Self {
cancellation_token: CancellationToken::new(),
task_handles: Vec::new(),
}
}

pub fn register_task<T>(&mut self, task: T)

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / linux_amd64

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / macos_arm64

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / linux_arm64

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / macos_amd64

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / cargo build

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / cargo test

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / benchmark / router

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / federation-audit

method `register_task` is never used

Check failure on line 27 in bin/router/src/background_tasks/mod.rs

View workflow job for this annotation

GitHub Actions / graphql-over-http

method `register_task` is never used
where
T: BackgroundTask + 'static,
{
info!("registering background task: {}", task.id());
let child_token = self.cancellation_token.clone();
let task_arc = Arc::new(task);

let handle = tokio::spawn(async move {
task_arc.run(child_token).await;
});

self.task_handles.push(handle);
}

pub async fn shutdown(self) {
info!("shutdown triggered, stopping all background tasks...");
self.cancellation_token.cancel();

debug!("waiting for background tasks to finish...");
join_all(self.task_handles).await;

println!("all background tasks have been shut down gracefully.");
Comment on lines +47 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are a couple of improvements that can be made here:

  1. Handle Task Panics: The result of join_all is currently ignored. If a background task panics, it will result in a JoinError which will be silently swallowed. It's important to handle these results and log any panics to aid in debugging.
  2. Consistent Logging: The println! macro is used, but the rest of the project uses the tracing crate. For consistency and to benefit from structured logging, it's better to use info!.

This suggestion addresses both points.

        let join_results = join_all(self.task_handles).await;
        for result in join_results {
            if let Err(err) = result {
                error!("A background task panicked during shutdown: {:?}", err);
            }
        }

        info!("all background tasks have been shut down gracefully.");

}
}
13 changes: 11 additions & 2 deletions bin/router/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod background_tasks;
mod http_utils;
mod logger;
mod pipeline;
Expand All @@ -6,6 +7,7 @@
use std::sync::Arc;

use crate::{
background_tasks::BackgroundTasksManager,
http_utils::{health::health_check_handler, landing_page::landing_page_handler},
logger::configure_logging,
pipeline::graphql_request_handler,
Expand All @@ -19,6 +21,7 @@
};

use hive_router_query_planner::utils::parsing::parse_schema;
use tracing::info;

async fn graphql_endpoint_handler(
mut request: HttpRequest,
Expand All @@ -37,8 +40,9 @@
let parsed_schema = parse_schema(&supergraph_sdl);
let addr = router_config.http.address();
let shared_state = RouterSharedState::new(parsed_schema, router_config);
let mut bg_tasks_manager = BackgroundTasksManager::new();

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / linux_amd64

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / macos_arm64

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / linux_arm64

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / macos_amd64

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / cargo build

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / cargo test

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / clippy

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / benchmark / router

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / federation-audit

variable does not need to be mutable

Check failure on line 43 in bin/router/src/lib.rs

View workflow job for this annotation

GitHub Actions / graphql-over-http

variable does not need to be mutable

web::HttpServer::new(move || {
let maybe_error = web::HttpServer::new(move || {
web::App::new()
.state(shared_state.clone())
.route("/graphql", web::to(graphql_endpoint_handler))
Expand All @@ -48,5 +52,10 @@
.bind(addr)?
.run()
.await
.map_err(|err| err.into())
.map_err(|err| err.into());

info!("server stopped, clearning background tasks");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There's a typo in the log message. "clearning" should be "cleaning".

Suggested change
info!("server stopped, clearning background tasks");
info!("server stopped, cleaning background tasks");

bg_tasks_manager.shutdown().await;

maybe_error
}
Loading