Add tests to udp::handlers

[josecelano]

I continue adding more tests to other parts of the app. Now the udp::handlers mod.

I've added one test for the connect request.

[josecelano]

I'm trying to add a second test for the same function:

pub async fn handle_connect(
    remote_addr: SocketAddr,
    request: &ConnectRequest,
    tracker: Arc<TorrentTracker>,
) -> Result<Response, ServerError> {
    let connection_id = get_connection_id(&remote_addr);

    let response = Response::from(ConnectResponse {
        transaction_id: request.transaction_id,
        connection_id,
    });

    // send stats event
    match remote_addr {
        SocketAddr::V4(_) => {
            tracker.send_stats_event(TrackerStatisticsEvent::Udp4Connect).await;
        }
        SocketAddr::V6(_) => {
            tracker.send_stats_event(TrackerStatisticsEvent::Udp6Connect).await;
        }
    }

    Ok(response)
}

I want to check that tracker.send_stats_event is called at least once.

I'm trying to use Mockall, but It seems you can not mock an async method even if you move it into a separate trait.

[josecelano]

hi @WarmBeer @da2ce7 , I've tried two things:

1. Mock TorrentTracker::send_stats_event to check that method is called once with the right event.
2. Inject the StatsTracker in the TorrentTracker constructor so I can mock the StatsTracker::send_event method.

In the first case, as I mentioned, you cannot mock an async method. In the second case, I do not know how to inject a mutable reference in a thread-safe way.

On the other hand, I was wondering why the method is async. The reason is that the underlying method is also async:

impl TorrentTracker {
    pub async fn get_stats(&self) -> RwLockReadGuard<'_, TrackerStatistics> {
        self.stats_tracker.get_stats().await
    }
}

impl StatsTracker {
    pub async fn send_event(&self, event: TrackerStatisticsEvent) -> Option<Result<(), SendError<TrackerStatisticsEvent>>> {
        if let Some(tx) = &self.channel_sender {
            Some(tx.send(event).await)
        } else {
            None
        }
    }
}

impl<T> Sender<T> {
    pub(crate) fn new(chan: chan::Tx<T, Semaphore>) -> Sender<T> {
        Sender { chan }
    }

    pub async fn send(&self, value: T) -> Result<(), SendError<T>> {
        match self.reserve().await {
            Ok(permit) => {
                permit.send(value);
                Ok(())
            }
            Err(_) => Err(SendError(value)),
        }
    }

but in the handle_connect function, we wait anyway:

pub async fn handle_connect(
    remote_addr: SocketAddr,
    request: &ConnectRequest,
    tracker: Arc<TorrentTracker>,
) -> Result<Response, ServerError> {
    let connection_id = get_connection_id(&remote_addr);

    let response = Response::from(ConnectResponse {
        transaction_id: request.transaction_id,
        connection_id,
    });

    // send stats event
    match remote_addr {
        SocketAddr::V4(_) => {
            tracker.send_stats_event(TrackerStatisticsEvent::Udp4Connect).await;
        }
        SocketAddr::V6(_) => {
            tracker.send_stats_event(TrackerStatisticsEvent::Udp6Connect).await;
        }
    }

    Ok(response)
}

We do not send the response until we receive the confirmation that the event has been sent.
Is that behaviour intentional? Maybe we could send the response and afterwards send the event to increase counters in statistics.

Something like:

pub fn handle_connect(
    &self,
    remote_addr: SocketAddr,
    request: &ConnectRequest,
    tracker: Arc<TorrentTracker>,
) -> Result<Response, ServerError> {
    let connection_id = get_connection_id(&remote_addr);

    let response = Response::from(ConnectResponse {
        transaction_id: request.transaction_id,
        connection_id,
    });

    // send stats event
    match remote_addr {
        SocketAddr::V4(_) => {
            self.trigger_event(TrackerStatisticsEvent::Udp4Connect);
        }
        SocketAddr::V6(_) => {
            self.trigger_event(TrackerStatisticsEvent::Udp6Connect);
        }
    }

    Ok(response)
}

The trigger_event method only collects events during the request process. After handling the request, the server can send all the events:

    pub async fn start(&self) {
        loop {
            let mut data = [0; MAX_PACKET_SIZE];
            let socket = self.socket.clone();
            let tracker = self.tracker.clone();
            let request_handler = self.request_handler.clone();

            tokio::select! {
                _ = tokio::signal::ctrl_c() => {
                    info!("Stopping UDP server: {}..", socket.local_addr().unwrap());
                    break;
                }
                Ok((valid_bytes, remote_addr)) = socket.recv_from(&mut data) => {
                    let payload = data[..valid_bytes].to_vec();

                    debug!("Received {} bytes from {}", payload.len(), remote_addr);
                    debug!("{:?}", payload);

                    let response = request_handler.handle(remote_addr, payload, tracker);

                    UdpServer::send_response(socket, remote_addr, response).await;

                    // We get the list of pending events, and we send them.
                    // Removing them from the list.  
                    tracker.send_events(request_handler.consume_events()).await
                }
            }
        }
    }

With this change:

• The response can be sent immediately.
• The response could be sent, but the statistics could not be updated consistently (if something fails after sending the response).
• The response should be faster, although increasing the counters is cheap now.
• We could easily mock the request_handler::trigger_event. We can create a trait for structs that raise events. We can apply the same solution to all tests that trigger events.

[mickvandijke]

Hi @josecelano ,

We do not send the response until we receive the confirmation that the event has been sent.
Is that behaviour intentional? Maybe we could send the response and afterwards send the event to increase counters in statistics.

This was done intentional. It only sends the event to a queue, so it should be pretty much instant. The events queue then gets processed by a separate stats worker thread.

With this change:

The response can be sent immediately.
The response could be sent, but the statistics could not be updated consistently (if something fails after sending the response).
The response should be faster, although increasing the counters is cheap now.
We could easily mock the request_handler::trigger_event. We can create a trait for structs that raise events. We can apply the same solution to all tests that trigger events.

I think this is a good improvement. 👍

[josecelano]

hi @da2ce7 @WarmBeer, I have added the missing tests we discussed this morning.

Finally, I didn't use a closure because the closure would be only the factory, but I needed a trait anyway to mock the final object in the test. The closure allows me only to inline the service instantiation.

The solution @WarmBeer told me seems to work. I've changed the TorrentTracker constructor:

pub struct TorrentTracker {
    pub config: Arc<Configuration>,
    ...
    stats_tracker: Box<dyn TrackerStatsService>,
    database: Box<dyn Database>,
    ...
}

impl TorrentTracker {
    pub fn new(config: Arc<Configuration>, stats_tracker: Box<dyn TrackerStatsService>) -> Result<TorrentTracker, r2d2::Error> {
        let database = database::connect_database(&config.db_driver, &config.db_path)?;

        Ok(TorrentTracker {
            config: config.clone(),
            mode: config.mode,
            keys: RwLock::new(std::collections::HashMap::new()),
            whitelist: RwLock::new(std::collections::HashSet::new()),
            torrents: RwLock::new(std::collections::BTreeMap::new()),
            stats_tracker,
            database,
        })
    }

The StatsTracker is used like the database, but it's instantiated outside.

I've also created my own mock for the StatsTracker. I did not want to add mockall yet. I called it TrackerStatsServiceMock.

I've segregated the responsibilities into two traits:

• TrackerStatisticsEventSender: it allows you to send events related to statistics.
• TrackerStatisticsRepository: it allows you to get statistics.

I joined both traits in the TrackerStatsService trait for convenience because I did not want to change the production code too much. But we can inject twice the same StatsTracker instance in the TorrentTrracker constructor. Anyway, in the future, I would prefer to have two or three different structs: event sender, event receiver, stats repo.

[josecelano]

hi @WarmBeer @da2ce7, I've merged the stacked PRs into this one:

• Add tests for udp::handlers:handle_scrape #93
• Add tests to udp::handlers:handle_announce  #91