diff --git a/rust/gui-client/src-common/src/controller.rs b/rust/gui-client/src-common/src/controller.rs index 5f7df8bc6..f62b5f370 100644 --- a/rust/gui-client/src-common/src/controller.rs +++ b/rust/gui-client/src-common/src/controller.rs @@ -1,7 +1,5 @@ use crate::{ - auth, deep_link, - errors::Error, - ipc, logging, + auth, deep_link, ipc, logging, settings::{self, AdvancedSettings}, system_tray::{self, Event as TrayMenuEvent}, updates, @@ -187,7 +185,7 @@ impl Controller { advanced_settings: AdvancedSettings, log_filter_reloader: FilterReloadHandle, updates_rx: mpsc::Receiver>, - ) -> Result<(), Error> { + ) -> Result<()> { tracing::debug!("Starting new instance of `Controller`"); let (ipc_client, ipc_rx) = ipc::Client::new().await?; @@ -218,7 +216,7 @@ impl Controller { Ok(()) } - pub async fn main_loop(mut self) -> Result<(), Error> { + pub async fn main_loop(mut self) -> Result<()> { self.update_telemetry_context().await?; if let Some(token) = self @@ -260,17 +258,19 @@ impl Controller { self.try_retry_connection().await? } EventloopTick::NetworkChanged(Err(e)) | EventloopTick::DnsChanged(Err(e)) => { - return Err(Error::Other(e)); + return Err(e); } - EventloopTick::IpcMsg(Some(Ok(msg))) => { + EventloopTick::IpcMsg(msg) => { + let msg = msg + .context("IPC closed")? + .context("Failed to read from IPC")?; + match self.handle_ipc_msg(msg).await? { ControlFlow::Break(()) => break, ControlFlow::Continue(()) => continue, }; } - EventloopTick::IpcMsg(Some(Err(e))) => return Err(Error::IpcRead(e)), - EventloopTick::IpcMsg(None) => return Err(Error::IpcClosed), EventloopTick::ControllerRequest(Some(req)) => self.handle_request(req).await?, EventloopTick::ControllerRequest(None) => { @@ -281,7 +281,7 @@ impl Controller { self.handle_update_notification(notification)? } EventloopTick::UpdateNotification(None) => { - return Err(Error::Other(anyhow!("Update checker task stopped"))); + return Err(anyhow!("Update checker task stopped")); } } } @@ -324,7 +324,7 @@ impl Controller { .await } - async fn start_session(&mut self, token: SecretString) -> Result<(), Error> { + async fn start_session(&mut self, token: SecretString) -> Result<()> { match self.status { Status::Disconnected | Status::RetryingConnection { .. } => {} Status::Quitting => Err(anyhow!("Can't connect to Firezone, we're quitting"))?, @@ -372,7 +372,7 @@ impl Controller { Ok(()) } - async fn handle_deep_link(&mut self, url: &SecretString) -> Result<(), Error> { + async fn handle_deep_link(&mut self, url: &SecretString) -> Result<()> { let auth_response = deep_link::parse_auth_callback(url).context("Couldn't parse scheme request")?; @@ -390,7 +390,7 @@ impl Controller { Ok(()) } - async fn handle_request(&mut self, req: ControllerRequest) -> Result<(), Error> { + async fn handle_request(&mut self, req: ControllerRequest) -> Result<()> { match req { Req::ApplySettings(settings) => { self.log_filter_reloader @@ -437,7 +437,7 @@ impl Controller { } Req::SchemeRequest(url) => match self.handle_deep_link(&url).await { Ok(()) => {} - Err(Error::Other(error)) + Err(error) if error .root_cause() .downcast_ref::() @@ -547,17 +547,16 @@ impl Controller { Ok(()) } - async fn handle_ipc_msg(&mut self, msg: IpcServerMsg) -> Result, Error> { + async fn handle_ipc_msg(&mut self, msg: IpcServerMsg) -> Result> { match msg { IpcServerMsg::ClearedLogs(result) => { let Some(tx) = self.clear_logs_callback.take() else { - return Err(Error::Other(anyhow!( + return Err(anyhow!( "Can't handle `IpcClearedLogs` when there's no callback waiting for a `ClearLogs` result" - ))); + )); }; - tx.send(result).map_err(|_| { - Error::Other(anyhow!("Couldn't send `ClearLogs` result to Tauri task")) - })?; + tx.send(result) + .map_err(|_| anyhow!("Couldn't send `ClearLogs` result to Tauri task"))?; } IpcServerMsg::ConnectResult(result) => { self.handle_connect_result(result).await?; @@ -627,10 +626,7 @@ impl Controller { Ok(ControlFlow::Continue(())) } - async fn handle_connect_result( - &mut self, - result: Result<(), IpcServiceError>, - ) -> Result<(), Error> { + async fn handle_connect_result(&mut self, result: Result<(), IpcServiceError>) -> Result<()> { let Status::WaitingForPortal { start_instant, token, diff --git a/rust/gui-client/src-common/src/errors.rs b/rust/gui-client/src-common/src/errors.rs deleted file mode 100644 index 17d59dd6e..000000000 --- a/rust/gui-client/src-common/src/errors.rs +++ /dev/null @@ -1,57 +0,0 @@ -use anyhow::Result; -use firezone_headless_client::ipc; - -pub const GENERIC_MSG: &str = "An unexpected error occurred. Please try restarting Firezone. If the issue persists, contact your administrator."; - -// TODO: Replace with `anyhow` gradually per -#[derive(Debug, thiserror::Error)] -pub enum Error { - #[error("IPC service not found")] - IpcNotFound, - #[error("IPC closed")] - IpcClosed, - #[error("IPC read failed")] - IpcRead(#[source] anyhow::Error), - #[error(transparent)] - Other(#[from] anyhow::Error), -} - -impl From for Error { - fn from(value: ipc::Error) -> Self { - match value { - ipc::Error::NotFound(_) => Self::IpcNotFound, - ipc::Error::Other(error) => Self::Other(error), - } - } -} - -impl Error { - // Decision to put the error strings here: - // This message gets shown to users in the GUI and could be localized, unlike - // messages in the log which only need to be used for `git grep`. - pub fn user_friendly_msg(&self) -> String { - match self { - Error::IpcNotFound => { - "Couldn't find Firezone IPC service. Is the service running?".to_string() - } - Error::IpcClosed => "IPC connection closed".to_string(), - Error::IpcRead(_) => "IPC read failure".to_string(), - Error::Other(_) => GENERIC_MSG.to_string(), - } - } -} - -/// Blocks the thread and shows an error dialog -/// -/// Doesn't play well with async, only use this if we're bailing out of the -/// entire process. -pub fn show_error_dialog(msg: String) -> Result<()> { - // I tried the Tauri dialogs and for some reason they don't show our - // app icon. - native_dialog::MessageDialog::new() - .set_title("Firezone Error") - .set_text(&msg) - .set_type(native_dialog::MessageType::Error) - .show_alert()?; - Ok(()) -} diff --git a/rust/gui-client/src-common/src/ipc.rs b/rust/gui-client/src-common/src/ipc.rs index 7888b4cdc..de11110e0 100644 --- a/rust/gui-client/src-common/src/ipc.rs +++ b/rust/gui-client/src-common/src/ipc.rs @@ -1,8 +1,5 @@ use anyhow::{Context as _, Result}; -use firezone_headless_client::{ - IpcClientMsg, - ipc::{self, Error}, -}; +use firezone_headless_client::{IpcClientMsg, ipc}; use futures::SinkExt; use secrecy::{ExposeSecret, SecretString}; use std::net::IpAddr; @@ -15,7 +12,7 @@ pub struct Client { } impl Client { - pub async fn new() -> Result<(Self, ipc::ClientRead), ipc::Error> { + pub async fn new() -> Result<(Self, ipc::ClientRead)> { tracing::debug!( client_pid = std::process::id(), "Connecting to IPC service..." @@ -38,19 +35,14 @@ impl Client { Ok(()) } - pub async fn connect_to_firezone( - &mut self, - api_url: &str, - token: SecretString, - ) -> Result<(), Error> { + pub async fn connect_to_firezone(&mut self, api_url: &str, token: SecretString) -> Result<()> { let token = token.expose_secret().clone(); self.send_msg(&IpcClientMsg::Connect { api_url: api_url.to_string(), token, }) .await - .context("Couldn't send Connect message") - .map_err(Error::Other)?; + .context("Couldn't send Connect message")?; Ok(()) } diff --git a/rust/gui-client/src-common/src/lib.rs b/rust/gui-client/src-common/src/lib.rs index d6bb886d2..b80831479 100644 --- a/rust/gui-client/src-common/src/lib.rs +++ b/rust/gui-client/src-common/src/lib.rs @@ -4,7 +4,6 @@ pub mod auth; pub mod compositor; pub mod controller; pub mod deep_link; -pub mod errors; pub mod ipc; pub mod logging; pub mod settings; diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index df645b269..ff8903a35 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -1,8 +1,9 @@ use anyhow::{Context as _, Result, bail}; use clap::{Args, Parser}; use firezone_gui_client_common::{ - self as common, controller::Failure, deep_link, errors, settings::AdvancedSettings, + self as common, controller::Failure, deep_link, settings::AdvancedSettings, }; +use firezone_headless_client::ipc; use firezone_telemetry::Telemetry; use tracing::instrument; use tracing_subscriber::EnvFilter; @@ -15,12 +16,6 @@ mod logging; mod settings; mod welcome; -#[derive(Debug, thiserror::Error)] -pub(crate) enum Error { - #[error("GUI module error: {0}")] - Gui(#[from] common::errors::Error), -} - /// The program's entry point, equivalent to `main` #[instrument(skip_all)] pub(crate) fn run() -> Result<()> { @@ -42,7 +37,7 @@ pub(crate) fn run() -> Result<()> { Ok(false) => bail!("The GUI should run as a normal user, not elevated"), #[cfg(not(target_os = "windows"))] // Windows elevation check never fails. Err(error) => { - common::errors::show_error_dialog(error.user_friendly_msg())?; + show_error_dialog(&error.user_friendly_msg())?; Err(error.into()) } } @@ -117,26 +112,27 @@ fn run_gui(cli: Cli) -> Result<()> { .find_map(|e| e.downcast_ref::()) .is_some_and(|e| matches!(e, tauri_runtime::Error::CreateWebview(_))) { - common::errors::show_error_dialog("Firezone cannot start because WebView2 is not installed. Follow the instructions at .".to_string())?; - return Err(anyhow); - } - - if anyhow.root_cause().is::() { - common::errors::show_error_dialog( - "Firezone is already running. If it's not responding, force-stop it." - .to_string(), + show_error_dialog( + "Firezone cannot start because WebView2 is not installed. Follow the instructions at .", )?; return Err(anyhow); } - // TODO: Get rid of `errors::Error` and check for sources individually like above. - if let Some(error) = anyhow.root_cause().downcast_ref::() { - common::errors::show_error_dialog(error.user_friendly_msg())?; - tracing::error!("GUI failed: {anyhow:#}"); + if anyhow.root_cause().is::() { + show_error_dialog( + "Firezone is already running. If it's not responding, force-stop it.", + )?; return Err(anyhow); } - common::errors::show_error_dialog(common::errors::GENERIC_MSG.to_owned())?; + if anyhow.root_cause().is::() { + show_error_dialog("Couldn't find Firezone IPC service. Is the service running?")?; + return Err(anyhow); + } + + show_error_dialog( + "An unexpected error occurred. Please try restarting Firezone. If the issue persists, contact your administrator.", + )?; tracing::error!("GUI failed: {anyhow:#}"); Err(anyhow) @@ -161,6 +157,21 @@ fn fix_log_filter(settings: &mut AdvancedSettings) -> Result<()> { Ok(()) } +/// Blocks the thread and shows an error dialog +/// +/// Doesn't play well with async, only use this if we're bailing out of the +/// entire process. +fn show_error_dialog(msg: &str) -> Result<()> { + // I tried the Tauri dialogs and for some reason they don't show our + // app icon. + native_dialog::MessageDialog::new() + .set_title("Firezone Error") + .set_text(msg) + .set_type(native_dialog::MessageType::Error) + .show_alert()?; + Ok(()) +} + /// Starts logging /// /// Don't drop the log handle or logging will stop. diff --git a/rust/headless-client/src/ipc_service/ipc.rs b/rust/headless-client/src/ipc_service/ipc.rs index 26c7bbc11..8ae2d604b 100644 --- a/rust/headless-client/src/ipc_service/ipc.rs +++ b/rust/headless-client/src/ipc_service/ipc.rs @@ -24,15 +24,9 @@ pub type ClientWrite = FramedWrite, Encoder, Decoder>; pub(crate) type ServerWrite = FramedWrite, Encoder>; -// pub so that the GUI can display a human-friendly message #[derive(Debug, thiserror::Error)] -pub enum Error { - #[error("Couldn't find IPC service `{0}`")] - NotFound(String), - - #[error("{0:#}")] // Use alternate display here to log entire chain of errors. - Other(anyhow::Error), -} +#[error("Couldn't find IPC service `{0}`")] +pub struct NotFound(String); /// A name that both the server and client can use to find each other /// @@ -128,7 +122,7 @@ impl tokio_util::codec::Encoder<&E> for Encoder { /// Connect to the IPC service /// /// Public because the GUI Client will need it -pub async fn connect_to_service(id: ServiceId) -> Result<(ClientRead, ClientWrite), Error> { +pub async fn connect_to_service(id: ServiceId) -> Result<(ClientRead, ClientWrite)> { // This is how ChatGPT recommended, and I couldn't think of any more clever // way before I asked it. let mut last_err = None; @@ -280,11 +274,4 @@ mod tests { } Ok(()) } - - #[test] - fn error_logs_all_anyhow_sources_on_display() { - let err = Error::Other(anyhow::anyhow!("foo").context("bar").context("baz")); - - assert_eq!(err.to_string(), "baz: bar: foo"); - } } diff --git a/rust/headless-client/src/ipc_service/ipc/linux.rs b/rust/headless-client/src/ipc_service/ipc/linux.rs index fa97dd41a..d1f76d635 100644 --- a/rust/headless-client/src/ipc_service/ipc/linux.rs +++ b/rust/headless-client/src/ipc_service/ipc/linux.rs @@ -1,4 +1,4 @@ -use super::{Error, ServiceId}; +use super::{NotFound, ServiceId}; use anyhow::{Context as _, Result}; use firezone_bin_shared::BUNDLE_ID; use std::{io::ErrorKind, os::unix::fs::PermissionsExt, path::PathBuf}; @@ -18,20 +18,18 @@ pub(crate) type ServerStream = UnixStream; /// Connect to the IPC service #[expect(clippy::wildcard_enum_match_arm)] -pub async fn connect_to_service(id: ServiceId) -> Result { +pub async fn connect_to_service(id: ServiceId) -> Result { let path = ipc_path(id); let stream = UnixStream::connect(&path) .await .map_err(|error| match error.kind() { - ErrorKind::NotFound => Error::NotFound(path.display().to_string()), - _ => Error::Other( - anyhow::Error::new(error).context("Couldn't connect to Unix domain socket"), - ), - })?; + ErrorKind::NotFound => anyhow::Error::new(NotFound(path.display().to_string())), + _ => anyhow::Error::new(error), + }) + .context("Couldn't connect to Unix domain socket")?; let cred = stream .peer_cred() - .context("Couldn't get PID of UDS server") - .map_err(Error::Other)?; + .context("Couldn't get PID of UDS server")?; tracing::debug!( uid = cred.uid(), gid = cred.gid(), diff --git a/rust/headless-client/src/ipc_service/ipc/windows.rs b/rust/headless-client/src/ipc_service/ipc/windows.rs index 8c1a08cbd..9a044fc64 100644 --- a/rust/headless-client/src/ipc_service/ipc/windows.rs +++ b/rust/headless-client/src/ipc_service/ipc/windows.rs @@ -1,4 +1,4 @@ -use super::{Error, ServiceId}; +use super::{NotFound, ServiceId}; use anyhow::{Context as _, Result, bail}; use firezone_bin_shared::BUNDLE_ID; use std::{ffi::c_void, io::ErrorKind, os::windows::io::AsRawHandle, time::Duration}; @@ -24,21 +24,22 @@ pub(crate) type ServerStream = named_pipe::NamedPipeServer; /// This is async on Linux #[expect(clippy::unused_async)] #[expect(clippy::wildcard_enum_match_arm)] -pub(crate) async fn connect_to_service(id: ServiceId) -> Result { +pub(crate) async fn connect_to_service(id: ServiceId) -> Result { let path = ipc_path(id); let stream = named_pipe::ClientOptions::new() .open(&path) .map_err(|error| match error.kind() { - ErrorKind::NotFound => Error::NotFound(path), - _ => Error::Other(error.into()), - })?; + ErrorKind::NotFound => anyhow::Error::new(NotFound(path)), + _ => anyhow::Error::new(error), + }) + .context("Couldn't connect to named pipe")?; let handle = HANDLE(stream.as_raw_handle()); let mut server_pid: u32 = 0; // SAFETY: Windows doesn't store this pointer or handle, and we just got the handle // from Tokio, so it should be valid. unsafe { GetNamedPipeServerProcessId(handle, &mut server_pid) } - .context("Couldn't get PID of named pipe server") - .map_err(Error::Other)?; + .context("Couldn't get PID of named pipe server")?; + tracing::debug!(?server_pid, "Made IPC connection"); Ok(stream) }