Skip to content

Refactor event listeners initialization#1476

Merged
josecelano merged 11 commits into
torrust:developfrom
josecelano:1444-overhaul-stats-refactor-event-listeners-initialization
Apr 25, 2025
Merged

Refactor event listeners initialization#1476
josecelano merged 11 commits into
torrust:developfrom
josecelano:1444-overhaul-stats-refactor-event-listeners-initialization

Conversation

@josecelano

@josecelano josecelano commented Apr 23, 2025

Copy link
Copy Markdown
Member

Move the spawning of new tasks for event listeners from app container instantiation to job execution.

This will enable us to easily add more event listeners in the future. Additionally, it enhances the way the tracker handles spawned tasks. We should have control over all JoinHandles for all tasks.

This promotes a cleaner task lifecycle. Some good practices:

  • There should always be an owner, responsible for the spawned tasks. No orphan tasks.
  • It should be possible to gracefully shut down the task.
  • The state of the task should be known.
  • Sometimes the owner needs to retrieve data (for example, status) from the task (when it starts or while it's active) or send the task additional commands.

Subtasks

  • HTTP Tracker Core event listener
    • Step 1: Separate factory from spawning task in the current setup::factory function.
    • Step 2: Split factory function into factory and running listener.
    • Step 3: Add Keeper to HttpTrackerCoreContainer (so we can run the listener later).
    • Step 4: Move task creation to app start.
    • Step 5. Start event listener in test environment.
  • UDP Tracker Core event listener (Step 1 to 4)
  • UDP Tracker Server event listener (Step 1 to 4)

NOTE: This PR will not include the refactor to allow multiple listeners. See #1385.

… Step 1

This is the first step in a bigger refactor to move the start of event listeners from app container instantiation to app start (jobs creation).
@josecelano
josecelano force-pushed the 1444-overhaul-stats-refactor-event-listeners-initialization branch from 734a23c to 17fb909 Compare April 23, 2025 19:04
@josecelano josecelano self-assigned this Apr 23, 2025
@josecelano josecelano added the Code Cleanup / Refactoring Tidying and Making Neat label Apr 23, 2025
@codecov

codecov Bot commented Apr 23, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 93.16456% with 27 lines in your changes missing coverage. Please review.

Project coverage is 84.48%. Comparing base (831f411) to head (6d50a78).
Report is 12 commits behind head on develop.

Files with missing lines Patch % Lines
packages/http-tracker-core/src/statistics/setup.rs 85.71% 0 Missing and 3 partials ⚠️
packages/udp-tracker-core/src/statistics/keeper.rs 87.50% 2 Missing and 1 partial ⚠️
packages/udp-tracker-core/src/statistics/setup.rs 85.71% 0 Missing and 3 partials ⚠️
...ckages/udp-tracker-server/src/statistics/keeper.rs 88.00% 2 Missing and 1 partial ⚠️
...ackages/udp-tracker-server/src/statistics/setup.rs 85.71% 0 Missing and 3 partials ⚠️
src/app.rs 94.11% 0 Missing and 3 partials ⚠️
packages/udp-tracker-server/src/environment.rs 94.59% 0 Missing and 2 partials ⚠️
...ckages/axum-http-tracker-server/src/environment.rs 95.00% 0 Missing and 1 partial ⚠️
packages/axum-http-tracker-server/src/server.rs 90.90% 0 Missing and 1 partial ⚠️
...um-http-tracker-server/src/v1/handlers/announce.rs 87.50% 0 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1476      +/-   ##
===========================================
+ Coverage    84.45%   84.48%   +0.03%     
===========================================
  Files          256      256              
  Lines        19670    19843     +173     
  Branches     19670    19843     +173     
===========================================
+ Hits         16612    16765     +153     
+ Misses        2764     2763       -1     
- Partials       294      315      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

We are moving the execution of the event listener from AppContainer
initialization to jobs start. However, in tests we still have to run it
manually if we need it.
@josecelano

josecelano commented Apr 24, 2025

Copy link
Copy Markdown
Member Author

Hi @da2ce7, I've completed the refactor for one of the three packages that have events. I will apply the same refactor to the other two.

As I was expecting, there is a problem shutting down the tracker when I wait for the event listener to finish because we don't send a halt message to that task. We can include the join handle in the jobs vector once we implement this issue.

We should also improve the shutdown message.

image

That message is only for axum servers. We should:

  • Show the socket address.
  • Stop the loop after 90 seconds.
  • Stop the loop when there are no connections.

It continues to display the message even if all Axum servers are successfully shut down, but the main tracker cannot stop because there are other pending jobs, such as event listeners, in this case. I will open a new issue to address this improvement. I think we should show the job that is preventing the tracker from shutting down if that task does not provide that feature, like the axum server. We should show something like:

!! shuting tracker !!
Waiting for the HTTP core event listener to finish ...

Every second or so.

Maybe we should add this at the main level for all jobs regardless of whether the jobs internally show other messages while shutting down (like axum server).

This is a draft, non-tested implementation from ChatGPT 4o:

use torrust_tracker_lib::app;
use tokio::time::{sleep, Duration};
use tokio::task::JoinHandle;
use std::sync::{Arc};
use std::sync::atomic::{AtomicBool, Ordering};

#[tokio::main]
async fn main() {
    let (_app_container, raw_jobs) = app::run().await;

    // Create flags for each job
    let mut job_flags = Vec::new();
    let mut wrapped_jobs = Vec::new();

    for (i, job) in raw_jobs.into_iter().enumerate() {
        let running = Arc::new(AtomicBool::new(true));
        let running_clone = running.clone();
        job_flags.push((format!("Job {}", i + 1), running.clone()));

        let handle: JoinHandle<()> = tokio::spawn(async move {
            job.await;
            running_clone.store(false, Ordering::SeqCst);
        });

        wrapped_jobs.push(handle);
    }

    // Spawn the status printer
    let status_handle = tokio::spawn({
        let job_flags = job_flags.clone();
        async move {
            loop {
                sleep(Duration::from_secs(5)).await;

                let still_running: Vec<_> = job_flags
                    .iter()
                    .filter(|(_, flag)| flag.load(Ordering::SeqCst))
                    .collect();

                if still_running.is_empty() {
                    break;
                }

                println!("!! shutting tracker !!");
                for (name, _) in still_running {
                    println!("Waiting for {name} to finish ...");
                }
                println!();
            }
        }
    });

    // Wait for CTRL+C
    tokio::signal::ctrl_c().await.unwrap();
    tracing::info!("Torrust tracker shutting down ...");

    // Wait for all job handles
    for handle in wrapped_jobs {
        let _ = handle.await;
    }

    // Wait for the status task to exit
    let _ = status_handle.await;

    tracing::info!("Torrust tracker successfully shutdown.");
}

@josecelano
josecelano force-pushed the 1444-overhaul-stats-refactor-event-listeners-initialization branch from 5773c1b to e13fb47 Compare April 24, 2025 16:10
@josecelano
josecelano force-pushed the 1444-overhaul-stats-refactor-event-listeners-initialization branch from e13fb47 to 07c5858 Compare April 25, 2025 08:31
@josecelano
josecelano marked this pull request as ready for review April 25, 2025 12:19
@josecelano
josecelano marked this pull request as draft April 25, 2025 13:08
@josecelano

Copy link
Copy Markdown
Member Author

ACK 6d50a78

@josecelano
josecelano marked this pull request as ready for review April 25, 2025 15:42
@josecelano
josecelano merged commit 4bb7a5a into torrust:develop Apr 25, 2025
@josecelano josecelano linked an issue Apr 25, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Code Cleanup / Refactoring Tidying and Making Neat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overhaul Stats: Refactor event listeners initialization

1 participant