Fix reconnections & add logging

mullvad · Jan 29, 2025 · 9371626 · 9371626
1 parent 2b6c5ba
commit 9371626
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 16 deletions.
diff --git a/talpid-routing/src/unix/android.rs b/talpid-routing/src/unix/android.rs
@@ -52,6 +52,7 @@ pub enum Error {
 static ROUTE_UPDATES_TX: Mutex<Option<UnboundedSender<NetworkState>>> = Mutex::new(None);
 
 /// Android route manager actor.
+#[derive(Debug)]
 pub struct RouteManagerImpl {
     network_state_updates: UnboundedReceiver<NetworkState>,
 
@@ -62,6 +63,7 @@ pub struct RouteManagerImpl {
     waiting_for_route: VecDeque<WaitingForRoutes>,
 }
 
+#[derive(Debug)]
 struct WaitingForRoutes {
     response_tx: oneshot::Sender<Result<(), Error>>,
     required_routes: HashSet<RequiredRoute>,
@@ -98,20 +100,21 @@ impl RouteManagerImpl {
                 }
 
                 route_update = self.network_state_updates.next().fuse() => {
+                    self.last_state = route_update;
+                    // TODO: Handle None (sender closed)
                     // check each waiting client if we have the routes they expect
                     for _ in 0..self.waiting_for_route.len() {
                         // oneshot senders consume themselves, so we need to take them out of the list
                         let Some(client) = self.waiting_for_route.pop_front() else { break };
 
                         if client.response_tx.is_canceled() {
                             // do nothing, drop the sender
-                        } else if has_routes(route_update.as_ref(), &client.required_routes) {
+                        } else if has_routes(self.last_state.as_ref(), &client.required_routes) {
                             let _ = client.response_tx.send(Ok(()));
                         } else {
                             self.waiting_for_route.push_back(client);
                         }
                     }
-                    self.last_state = route_update;
                 }
             }
         }
@@ -125,6 +128,8 @@ impl RouteManagerImpl {
                 return ControlFlow::Break(());
             }
             RouteManagerCommand::AddRoutes(required_routes, response_tx) => {
+                log::info!("Current state: {self:#?}");
+                log::info!("Looking for deez routes: {required_routes:#?}");
                 if has_routes(self.last_state.as_ref(), &required_routes) {
                     let _ = response_tx.send(Ok(()));
                 } else {
@@ -133,7 +138,9 @@ impl RouteManagerImpl {
             }
             RouteManagerCommand::ClearRoutes => {
                 // The VPN tunnel is gone. We can't assume that any (desired) routes are up at this point.
-                self.last_state = None;
+                // TODO: This won't work right away, as we're apparently clearing routes when reconnecting ..
+                // self.last_state = None;
+                log::debug!("Clearing routes");
             },
         }
 

diff --git a/talpid-wireguard/src/lib.rs b/talpid-wireguard/src/lib.rs
@@ -475,18 +475,6 @@ impl WireguardMonitor {
                 .await;
 
             // Wait for routes to come up
-            // TODO: Time out (eventually) and return proper error
-            // let route_update =
-            //     route_updates.any(|routes_are_correct| async move { routes_are_correct });
-            // if let Err(_) =
-            //     tokio::time::timeout(std::time::Duration::from_secs(4), route_update).await
-            // {
-            //     // TODO: Wrong error. Expose "routes are not up" error
-            //     return Err(CloseMsg::SetupError(Error::SetupRoutingError(
-            //         talpid_routing::Error::RouteManagerDown,
-            //     )));
-            // }
-
             let routes_to_wait_for: std::collections::HashSet<RequiredRoute> = args
                 .tun_provider
                 .lock()
@@ -497,12 +485,21 @@ impl WireguardMonitor {
                 .copied()
                 .map(RequiredRoute::new)
                 .collect();
+
             args.route_manager
                 .add_routes(routes_to_wait_for)
                 .await
-                // TODO: Return a proper error
+                // TODO: Do not unwrap
                 .unwrap();
 
+            // if tokio::time::timeout(std::time::Duration::from_secs(4), route_update).await.is_err()
+            // {
+            //     // TODO: Wrong error. Expose "routes are not up" error
+            //     return Err(CloseMsg::SetupError(Error::SetupRoutingError(
+            //         talpid_routing::Error::RouteManagerDown,
+            //     )));
+            // }
+
             if should_negotiate_ephemeral_peer {
                 let ephemeral_obfs_sender = close_obfs_sender.clone();