Skip to content

Overhaul Stats: Refactor event listeners initialization #1444

Description

@josecelano

All the packages using events have a function to initialize statistics:

pub fn factory(tracker_usage_statistics: bool) -> (Option<Box<dyn event::sender::Sender>>, statistics::repository::Repository) {
    let mut keeper = statistics::keeper::Keeper::new();

    let opt_event_sender: Option<Box<dyn event::sender::Sender>> = if tracker_usage_statistics {
        let broadcaster = Broadcaster::default();

        keeper.run_event_listener(broadcaster.subscribe());

        Some(Box::new(broadcaster))
    } else {
        None
    };

    (opt_event_sender, keeper.repository)
}

That example is from the UDP tracker server package.

The problem is this factory method is called when the AppContainer is initialized in:

impl AppContainer {
    #[instrument(skip(configuration))]
    pub fn initialize(configuration: &Configuration) -> AppContainer {
        // ...
    }
}

And the AppContainer is initialized in the app setup:

pub async fn run() -> (Arc<AppContainer>, Vec<JoinHandle<()>>, Registar) {
    let (config, app_container) = bootstrap::app::setup(); // <- AppContainer initialized here

    let app_container = Arc::new(app_container);

    let (jobs, registar) = start(&config, &app_container).await;

    (app_container, jobs, registar)
}

I think the app::setup should only create the services (constructors) but not run any service, including spawning tasks.

I think we should call the keeper.run_event_listener(broadcaster.subscribe()); function in the start function where we start the jobs.

I think that the separation between building and running is a better design.

One concrete example of why I think this would be good is in the Keeper::run_event_listener:

pub struct Keeper {
    pub repository: Repository,
}

impl Default for Keeper {
    fn default() -> Self {
        Self::new()
    }
}

impl Keeper {
    #[must_use]
    pub fn new() -> Self {
        Self {
            repository: Repository::new(),
        }
    }

    pub fn run_event_listener(&mut self, receiver: Receiver<Event>) {
        let stats_repository = self.repository.clone();

        tokio::spawn(async move { dispatch_events(receiver, stats_repository).await });
    }
}

The task for the event listener is spawned during the app setup (building services), and it's not an async function. If the event dispatcher panics (function dispatch_events) there is no way to await for the returned value and handle it properly.

I think that is what is happening in this bug.

These tasks should also be included as jobs in the start function so we can wait for them to finish. If they panic (or finish) the main program can do something like send a notification or retry to launch the listener, ...

Metadata

Metadata

Assignees

Type

Fields

No fields configured for Task.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions