github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(gui-client): simplify error handling (#8519)

Browse Source 
As a follow-up from #7959, we can now simplify the error handling a fair
bit as all codepaths that can fail in the client are threaded back to
the main function.
This commit is contained in:
Thomas Eizinger
2025-03-27 08:39:26 +11:00
committed by GitHub
parent 95d3f765f4
commit f13234955a
8 changed files with 74 additions and 147 deletions
Download Patch File Download Diff File Expand all files Collapse all files

44
rust/gui-client/src-common/src/controller.rs
View File

@@ -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<I: GuiIntegration> Controller<I> {
advanced_settings: AdvancedSettings,
log_filter_reloader: FilterReloadHandle,
updates_rx: mpsc::Receiver<Option<updates::Notification>>,
) -> Result<(), Error> {
) -> Result<()> {
tracing::debug!("Starting new instance of `Controller`");
let (ipc_client, ipc_rx) = ipc::Client::new().await?;
@@ -218,7 +216,7 @@ impl<I: GuiIntegration> Controller<I> {
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<I: GuiIntegration> Controller<I> {
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<I: GuiIntegration> Controller<I> {
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<I: GuiIntegration> Controller<I> {
.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<I: GuiIntegration> Controller<I> {
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<I: GuiIntegration> Controller<I> {
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<I: GuiIntegration> Controller<I> {
}
Req::SchemeRequest(url) => match self.handle_deep_link(&url).await {
Ok(()) => {}
Err(Error::Other(error))
Err(error)
if error
.root_cause()
.downcast_ref::<auth::Error>()
@@ -547,17 +547,16 @@ impl<I: GuiIntegration> Controller<I> {
Ok(())
}
async fn handle_ipc_msg(&mut self, msg: IpcServerMsg) -> Result<ControlFlow<()>, Error> {
async fn handle_ipc_msg(&mut self, msg: IpcServerMsg) -> Result<ControlFlow<()>> {
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<I: GuiIntegration> Controller<I> {
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,

57
rust/gui-client/src-common/src/errors.rs
View File

@@ -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 <https://github.com/firezone/firezone/pull/3546#discussion_r1477114789>
#[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<ipc::Error> 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: <https://github.com/firezone/firezone/pull/3464#discussion_r1473608415>
// 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(())
}

16
rust/gui-client/src-common/src/ipc.rs
View File

@@ -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(())
}

1
rust/gui-client/src-common/src/lib.rs
View File

@@ -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;

53
rust/gui-client/src-tauri/src/client.rs
View File

@@ -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::<tauri_runtime::Error>())
.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 <https://www.firezone.dev/kb/client-apps/windows-gui-client>.".to_string())?;
return Err(anyhow);
}
if anyhow.root_cause().is::<deep_link::CantListen>() {
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 <https://www.firezone.dev/kb/client-apps/windows-gui-client>.",
)?;
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::<errors::Error>() {
common::errors::show_error_dialog(error.user_friendly_msg())?;
tracing::error!("GUI failed: {anyhow:#}");
if anyhow.root_cause().is::<deep_link::CantListen>() {
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::<ipc::NotFound>() {
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.

19
rust/headless-client/src/ipc_service/ipc.rs
View File

@@ -24,15 +24,9 @@ pub type ClientWrite = FramedWrite<WriteHalf<ClientStream>, Encoder<IpcClientMsg
pub(crate) type ServerRead = FramedRead<ReadHalf<ServerStream>, Decoder<IpcClientMsg>>;
pub(crate) type ServerWrite = FramedWrite<WriteHalf<ServerStream>, Encoder<IpcServerMsg>>;
// 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<E: serde::Serialize> tokio_util::codec::Encoder<&E> for Encoder<E> {
/// 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");
}
}

16
rust/headless-client/src/ipc_service/ipc/linux.rs
View File

@@ -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<ClientStream, Error> {
pub async fn connect_to_service(id: ServiceId) -> Result<ClientStream> {
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(),

15
rust/headless-client/src/ipc_service/ipc/windows.rs
View File

@@ -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<ClientStream, Error> {
pub(crate) async fn connect_to_service(id: ServiceId) -> Result<ClientStream> {
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)
}