From a822238205ae3e98fba6389dfd6002ccb327db53 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Tue, 6 Feb 2024 10:24:11 -0600 Subject: [PATCH] fix(windows): keep the main loop running even if a request has an error (#3569) Closes #3521 because now almost all of the `?` in the main loop will log an error and not bail. This is nice for trivial things like copying a resource when we're not signed in. (e.g. User opens the menu, the token expires, user clicks something, it should just do nothing instead of crashing) Closes a long-standing TODO for moving more of the request handling code inside `impl Controller` --- .../src-tauri/src/client/gui.rs | 244 +++++++++--------- 1 file changed, 124 insertions(+), 120 deletions(-) diff --git a/rust/windows-client/src-tauri/src/client/gui.rs b/rust/windows-client/src-tauri/src/client/gui.rs index 5425aa6be..26fa3f16a 100644 --- a/rust/windows-client/src-tauri/src/client/gui.rs +++ b/rust/windows-client/src-tauri/src/client/gui.rs @@ -481,41 +481,6 @@ struct Session { } impl Controller { - async fn new( - app: tauri::AppHandle, - ctlr_tx: CtlrTx, - logging_handles: client::logging::Handles, - advanced_settings: AdvancedSettings, - notify_controller: Arc, - ) -> Result { - let device_id = client::device_id::device_id(&app.config().tauri.bundle.identifier).await?; - - let mut this = Self { - advanced_settings, - app, - // Uses `std::fs` - auth: client::auth::Auth::new()?, - ctlr_tx, - session: None, - device_id, - logging_handles, - notify_controller, - tunnel_ready: false, - }; - - // Uses `std::fs` - if let Some(token) = this.auth.token()? { - // Connect immediately if we reloaded the token - if let Err(e) = this.start_session(token) { - tracing::error!("couldn't restart session on app start: {e:#?}"); - } - } else { - tracing::info!("No token / actor_name on disk, starting in signed-out state"); - } - - Ok(this) - } - // TODO: Figure out how re-starting sessions automatically will work /// Pre-req: the auth module must be signed in fn start_session(&mut self, token: SecretString) -> Result<()> { @@ -584,6 +549,92 @@ impl Controller { Ok(()) } + async fn handle_request(&mut self, req: ControllerRequest) -> Result<()> { + match req { + Req::Disconnected => { + tracing::debug!("connlib disconnected, tearing down Session"); + self.tunnel_ready = false; + if let Some(mut session) = self.session.take() { + tracing::info!("disconnecting connlib"); + // This is probably redundant since connlib shuts itself down if it's disconnected. + session.connlib.disconnect(None); + } + self.refresh_system_tray_menu()?; + } + Req::DisconnectedTokenExpired => { + tracing::info!("Token expired"); + self.sign_out()?; + show_notification( + "Firezone disconnected", + "To access resources, sign in again.", + )?; + } + Req::ExportLogs { path, stem } => logging::export_logs_to(path, stem).await?, + Req::GetAdvancedSettings(tx) => { + tx.send(self.advanced_settings.clone()).ok(); + } + Req::SchemeRequest(url) => self + .handle_deep_link(&url) + .await + .context("Couldn't handle deep link")?, + Req::SystemTrayMenu(TrayMenuEvent::ToggleWindow(window)) => { + self.toggle_window(window)? + } + Req::SystemTrayMenu(TrayMenuEvent::CancelSignIn | TrayMenuEvent::SignOut) => { + tracing::info!("User signed out or canceled sign-in"); + self.sign_out()?; + } + Req::SystemTrayMenu(TrayMenuEvent::Resource { id }) => self + .copy_resource(&id) + .context("Couldn't copy resource to clipboard")?, + Req::SystemTrayMenu(TrayMenuEvent::SignIn) => { + if let Some(req) = self.auth.start_sign_in()? { + let url = req.to_url(&self.advanced_settings.auth_base_url); + self.refresh_system_tray_menu()?; + tauri::api::shell::open( + &self.app.shell_scope(), + &url.expose_secret().inner, + None, + )?; + } + } + Req::SystemTrayMenu(TrayMenuEvent::Quit) => { + bail!("Impossible error: `Quit` should be handled before this") + } + Req::TunnelReady => { + self.tunnel_ready = true; + self.refresh_system_tray_menu()?; + + show_notification( + "Firezone connected", + "You are now signed in and able to access resources.", + )?; + } + Req::UpdateAvailable(release) => { + let title = format!("Firezone {} available for download", release.tag_name); + + // We don't need to route through the controller here either, we could + // use the `open` crate directly instead of Tauri's wrapper + // `tauri::api::shell::open` + show_clickable_notification( + &title, + "Click here to download the new version.", + self.ctlr_tx.clone(), + Req::UpdateNotificationClicked(release), + )?; + } + Req::UpdateNotificationClicked(release) => { + tracing::info!("UpdateNotificationClicked in run_controller!"); + tauri::api::shell::open( + &self.app.shell_scope(), + release.browser_download_url, + None, + )?; + } + } + Ok(()) + } + /// Returns a new system tray menu fn build_system_tray_menu(&self) -> tauri::SystemTrayMenu { // TODO: Refactor this and the auth module so that "Are we logged in" @@ -668,28 +719,48 @@ async fn run_controller( advanced_settings: AdvancedSettings, notify_controller: Arc, ) -> Result<()> { - let mut controller = Controller::new( - app.clone(), - ctlr_tx, - logging_handles, - advanced_settings, - notify_controller, - ) - .await - .context("couldn't create Controller")?; + let device_id = client::device_id::device_id(&app.config().tauri.bundle.identifier) + .await + .context("Failed to read / create device ID")?; - let mut have_internet = network_changes::check_internet()?; + let mut controller = Controller { + advanced_settings, + app, + auth: client::auth::Auth::new().context("Failed to set up auth module")?, + ctlr_tx, + session: None, + device_id, + logging_handles, + notify_controller, + tunnel_ready: false, + }; + + if let Some(token) = controller + .auth + .token() + .context("Failed to load token from disk during app start")? + { + controller + .start_session(token) + .context("Failed to restart session during app start")?; + } else { + tracing::info!("No token / actor_name on disk, starting in signed-out state"); + } + + let mut have_internet = + network_changes::check_internet().context("Failed initial check for internet")?; tracing::info!(?have_internet); - let mut com_worker = network_changes::Worker::new()?; + let mut com_worker = + network_changes::Worker::new().context("Failed to listen for network changes")?; loop { tokio::select! { - () = controller.notify_controller.notified() => if let Err(e) = controller.refresh_system_tray_menu() { - tracing::error!("couldn't reload resource list: {e:#?}"); + () = controller.notify_controller.notified() => if let Err(error) = controller.refresh_system_tray_menu() { + tracing::error!(?error, "Failed to reload resource list"); }, () = com_worker.notified() => { - let new_have_internet = network_changes::check_internet()?; + let new_have_internet = network_changes::check_internet().context("Failed to check for internet")?; if new_have_internet != have_internet { have_internet = new_have_internet; // TODO: Stop / start / restart connlib as needed here @@ -697,78 +768,11 @@ async fn run_controller( } }, req = rx.recv() => { - let Some(req) = req else { - break; - }; match req { - Req::Disconnected => { - tracing::debug!("connlib disconnected, tearing down Session"); - controller.tunnel_ready = false; - if let Some(mut session) = controller.session.take() { - tracing::debug!("disconnecting connlib"); - // This is probably redundant since connlib shuts itself down if it's disconnected. - session.connlib.disconnect(None); - } - controller.refresh_system_tray_menu()?; - } - Req::DisconnectedTokenExpired => { - tracing::info!("Token expired"); - controller.sign_out()?; - show_notification("Firezone disconnected", "To access resources, sign in again.")?; - } - Req::ExportLogs{path, stem} => logging::export_logs_to(path, stem).await?, - Req::GetAdvancedSettings(tx) => { - tx.send(controller.advanced_settings.clone()).ok(); - } - Req::SchemeRequest(url) => if let Err(e) = controller.handle_deep_link(&url).await { - tracing::error!("couldn't handle deep link: {e:#?}"); - } - Req::SystemTrayMenu(TrayMenuEvent::ToggleWindow(window)) => controller.toggle_window(window)?, - Req::SystemTrayMenu(TrayMenuEvent::CancelSignIn | TrayMenuEvent::SignOut) => { - tracing::info!("User signed out or canceled sign-in"); - controller.sign_out()?; - } - Req::SystemTrayMenu(TrayMenuEvent::Resource { id }) => if let Err(e) = controller.copy_resource(&id) { - tracing::error!("couldn't copy resource to clipboard: {e:#?}"); - } - Req::SystemTrayMenu(TrayMenuEvent::SignIn) => { - if let Some(req) = controller.auth.start_sign_in()? { - let url = req.to_url(&controller.advanced_settings.auth_base_url); - controller.refresh_system_tray_menu()?; - tauri::api::shell::open( - &app.shell_scope(), - &url.expose_secret().inner, - None, - )?; - } - } - Req::SystemTrayMenu(TrayMenuEvent::Quit) => break, - Req::TunnelReady => { - controller.tunnel_ready = true; - controller.refresh_system_tray_menu()?; - - show_notification("Firezone connected", "You are now signed in and able to access resources.")?; - } - Req::UpdateAvailable(release) => { - let title = format!("Firezone {} available for download", release.tag_name); - - // We don't need to route through the controller here either, we could - // use the `open` crate directly instead of Tauri's wrapper - // `tauri::api::shell::open` - show_clickable_notification( - &title, - "Click here to download the new version.", - controller.ctlr_tx.clone(), - Req::UpdateNotificationClicked(release), - )?; - } - Req::UpdateNotificationClicked(release) => { - tracing::info!("UpdateNotificationClicked in run_controller!"); - tauri::api::shell::open( - &app.shell_scope(), - release.browser_download_url, - None, - )?; + None => break, + Some(Req::SystemTrayMenu(TrayMenuEvent::Quit)) => break, + Some(req) => if let Err(error) = controller.handle_request(req).await { + tracing::error!(?error, "Failed to handle a ControllerRequest"); } } }