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
```
This commit is contained in:
Reactor Scram
2024-07-03 20:55:30 +00:00
committed by GitHub
parent 3b0f54ec3c
commit ecb38dedf9
4 changed files with 51 additions and 25 deletions

View File

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

View File

@@ -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<Self, super::Error> {
/// On Windows this uses async because of #5143 and #5566.
#[allow(clippy::unused_async)]
pub(crate) async fn new() -> Result<Self, super::Error> {
let path = sock_path()?;
let dir = path
.parent()

View File

@@ -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<Self, super::Error> {
pub(crate) async fn new() -> Result<Self, super::Error> {
// 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<named_pipe::NamedPipeServer, super::Error> {
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<named_pipe::NamedPipeServer, super::Error> {
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();

View File

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