From ecb38dedf99ff28177deecbaabfe00832af6234b Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Wed, 3 Jul 2024 20:55:30 +0000 Subject: [PATCH] fix(gui-client/windows): retry 10 times while creating the deep link server (#5570) Temporary fix for #5566 A better fix would be to merge the deep link and IPC service code, but I tried that a couple times and failed, their interfaces are different. ```[tasklist] ### Tasks - [x] Expand comment explaining the root cause - [x] Re-request review ``` --- .../src-tauri/src/client/deep_link.rs | 6 ++- .../src-tauri/src/client/deep_link/linux.rs | 4 +- .../src-tauri/src/client/deep_link/windows.rs | 46 +++++++++++++------ rust/gui-client/src-tauri/src/client/gui.rs | 20 ++++---- 4 files changed, 51 insertions(+), 25 deletions(-) diff --git a/rust/gui-client/src-tauri/src/client/deep_link.rs b/rust/gui-client/src-tauri/src/client/deep_link.rs index e4c49e4e0..04e97649e 100644 --- a/rust/gui-client/src-tauri/src/client/deep_link.rs +++ b/rust/gui-client/src-tauri/src/client/deep_link.rs @@ -4,7 +4,7 @@ // and named pipes on Windows, so TODO de-dupe the IPC code use crate::client::auth::Response as AuthResponse; -use anyhow::{bail, Context, Result}; +use anyhow::{bail, Context as _, Result}; use secrecy::{ExposeSecret, SecretString}; use url::Url; @@ -143,7 +143,9 @@ mod tests { /// Will fail with permission error if Firezone already ran as sudo #[tokio::test] async fn socket_smoke_test() -> Result<()> { - let server = super::Server::new().context("Couldn't start Server")?; + let server = super::Server::new() + .await + .context("Couldn't start Server")?; let server_task = tokio::spawn(async move { let bytes = server.accept().await?; Ok::<_, anyhow::Error>(bytes) diff --git a/rust/gui-client/src-tauri/src/client/deep_link/linux.rs b/rust/gui-client/src-tauri/src/client/deep_link/linux.rs index 97175aa43..54344096e 100644 --- a/rust/gui-client/src-tauri/src/client/deep_link/linux.rs +++ b/rust/gui-client/src-tauri/src/client/deep_link/linux.rs @@ -23,7 +23,9 @@ impl Server { /// Create a new deep link server to make sure we're the only instance /// /// Still uses `thiserror` so we can catch the deep_link `CantListen` error - pub(crate) fn new() -> Result { + /// On Windows this uses async because of #5143 and #5566. + #[allow(clippy::unused_async)] + pub(crate) async fn new() -> Result { let path = sock_path()?; let dir = path .parent() diff --git a/rust/gui-client/src-tauri/src/client/deep_link/windows.rs b/rust/gui-client/src-tauri/src/client/deep_link/windows.rs index 86013ce1e..1e59dd493 100644 --- a/rust/gui-client/src-tauri/src/client/deep_link/windows.rs +++ b/rust/gui-client/src-tauri/src/client/deep_link/windows.rs @@ -5,7 +5,7 @@ use super::FZ_SCHEME; use anyhow::{Context, Result}; use connlib_shared::BUNDLE_ID; use secrecy::Secret; -use std::{io, path::Path}; +use std::{io, path::Path, time::Duration}; use tokio::{io::AsyncReadExt, io::AsyncWriteExt, net::windows::named_pipe}; /// A server for a named pipe, so we can receive deep links from other instances @@ -19,21 +19,12 @@ impl Server { /// /// Panics if there is no Tokio runtime /// Still uses `thiserror` so we can catch the deep_link `CantListen` error - pub(crate) fn new() -> Result { + pub(crate) async fn new() -> Result { // This isn't air-tight - We recreate the whole server on each loop, // rather than binding 1 socket and accepting many streams like a normal socket API. - // I can only assume Tokio is following Windows' underlying API. - - // We could instead pick an ephemeral TCP port and write that to a file, - // akin to how Unix processes will write their PID to a file to manage long-running instances - // But this doesn't require us to listen on TCP. - - let mut server_options = named_pipe::ServerOptions::new(); - server_options.first_pipe_instance(true); - - let server = server_options - .create(pipe_path()) - .map_err(|_| super::Error::CantListen)?; + // Tokio appears to be following Windows' underlying API here, so not + // much we can do until Unix domain sockets have wide support in Windows. + let server = bind_to_pipe(&pipe_path()).await?; tracing::debug!("server is bound"); Ok(Server { inner: server }) @@ -66,6 +57,33 @@ impl Server { } } +async fn bind_to_pipe(pipe_path: &str) -> Result { + const NUM_ITERS: usize = 10; + // Relating to #5143 and #5566, sometimes re-creating a named pipe server + // in a loop fails. This is copied from `firezone_headless_client::ipc_service::ipc::windows`. + for i in 0..NUM_ITERS { + match create_pipe_server(pipe_path) { + Ok(server) => return Ok(server), + Err(super::Error::CantListen) => { + tracing::warn!("`create_pipe_server` failed, sleeping... (loop {i})"); + tokio::time::sleep(Duration::from_secs(1)).await; + } + Err(error) => Err(error)?, + } + } + Err(super::Error::CantListen) +} + +fn create_pipe_server(pipe_path: &str) -> Result { + let mut server_options = named_pipe::ServerOptions::new(); + server_options.first_pipe_instance(true); + + let server = server_options + .create(pipe_path) + .map_err(|_| super::Error::CantListen)?; + Ok(server) +} + /// Open a deep link by sending it to the already-running instance of the app pub async fn open(url: &url::Url) -> Result<()> { let path = pipe_path(); diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index e9c0c6857..8e7d24766 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -81,6 +81,11 @@ pub(crate) fn run( let rt = tokio::runtime::Runtime::new().context("Couldn't start Tokio runtime")?; let _guard = rt.enter(); + // Make sure we're single-instance + // We register our deep links to call the `open-deep-link` subcommand, + // so if we're at this point, we know we've been launched manually + let deep_link_server = rt.block_on(async { deep_link::Server::new().await })?; + let (ctlr_tx, ctlr_rx) = mpsc::channel(5); let managed = Managed { @@ -151,11 +156,6 @@ pub(crate) fn run( } }); - // Make sure we're single-instance - // We register our deep links to call the `open-deep-link` subcommand, - // so if we're at this point, we know we've been launched manually - let server = deep_link::Server::new()?; - if let Some(client::Cmd::SmokeTest) = &cli.command { let ctlr_tx = ctlr_tx.clone(); tokio::spawn(async move { @@ -171,7 +171,7 @@ pub(crate) fn run( // The single-instance check is done, so register our exe // to handle deep links deep_link::register().context("Failed to register deep link handler")?; - tokio::spawn(accept_deep_links(server, ctlr_tx.clone())); + tokio::spawn(accept_deep_links(deep_link_server, ctlr_tx.clone())); } if let Some(failure) = cli.fail_on_purpose() { @@ -380,7 +380,7 @@ async fn accept_deep_links(mut server: deep_link::Server, ctlr_tx: CtlrTx) -> Re Err(error) => tracing::error!(?error, "error while accepting deep link"), } // We re-create the named pipe server every time we get a link, because of an oddity in the Windows API. - server = deep_link::Server::new()?; + server = deep_link::Server::new().await?; } } @@ -535,7 +535,11 @@ impl Controller { Req::GetAdvancedSettings(tx) => { tx.send(self.advanced_settings.clone()).ok(); } - Req::SchemeRequest(url) => self.handle_deep_link(&url).await?, + Req::SchemeRequest(url) => { + if let Err(error) = self.handle_deep_link(&url).await { + tracing::error!(?error, "`handle_deep_link` failed"); + } + } Req::SignIn | Req::SystemTrayMenu(TrayMenuEvent::SignIn) => { if let Some(req) = self .auth