library: don't hide errors

[bryanlarsen]

Describe the feature

When wstunnel is used as a library and a tunnel fails, the user should get a Response::Err rather than a Response::Ok.

Describe the reason for such feature

• ergonomics
• being able to retry with slightly different connection parameters on failure

Currently wstunnel calls tracing::error! on most errors and then swallows the error rather than returning them to the original caller.

Describe alternatives you've considered

diff --git a/src/lib.rs b/src/lib.rs
index e05fa16..3201a88 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -38,25 +38,19 @@ use tokio::task::JoinSet;
 use tracing::{error, info};
 use url::Url;
 
-pub async fn run_client(args: Client, executor: impl TokioExecutor) -> anyhow::Result<()> {
-    let tunnels = create_client_tunnels(args, executor.clone()).await?;
-
-    // Start all tunnels
-    let (tx, rx) = oneshot::channel();
-    executor.spawn(async move {
-        let _ = JoinSet::from_iter(tunnels).join_all().await;
-        let _ = tx.send(());
-    });
+pub async fn run_client(args: Client, executor: impl TokioExecutor) -> Vec<anyhow::Result<()>> {
+    let tunnels = match create_client_tunnels(args, executor.clone()).await {
+        Ok(tunnels) => tunnels,
+        Err(err) => return vec![Err(err)],  // FIXME: this is probably unacceptable
+    };
 
-    // wait for all tunnels to finish
-    rx.await?;
-    Ok(())
+    JoinSet::from_iter(tunnels).join_all().await
 }
 
 async fn create_client_tunnels(
     args: Client,
     executor: impl TokioExecutor,
-) -> anyhow::Result<Vec<BoxFuture<'static, ()>>> {
+) -> anyhow::Result<Vec<BoxFuture<'static, anyhow::Result<()>>>> {
     let (tls_certificate, tls_key) = if let (Some(cert), Some(key)) =
         (args.tls_certificate.as_ref(), args.tls_private_key.as_ref())
     {
@@ -159,7 +153,8 @@ async fn create_client_tunnels(
     info!("Starting wstunnel client v{}", env!("CARGO_PKG_VERSION"),);
 
     // Keep track of all spawned tunnels
-    let mut tunnels: Vec<BoxFuture<()>> = Vec::with_capacity(args.remote_to_local.len() + args.local_to_remote.len());
+    let mut tunnels: Vec<BoxFuture<anyhow::Result<()>>> =
+        Vec::with_capacity(args.remote_to_local.len() + args.local_to_remote.len());
     macro_rules! spawn_tunnel {
         ( $($s:stmt);* ) => {
             tunnels.push(Box::pin(async move {
@@ -188,9 +183,7 @@ async fn create_client_tunnels(
                         host,
                         port,
                     };
-                    if let Err(err) = client.run_reverse_tunnel(remote, tcp_connector).await {
-                        error!("{:?}", err);
-                    }
+                    client.run_reverse_tunnel(remote, tcp_connector).await
                 }
             }

and so on.

A similar "print an swallow the error pattern is at https://github.com/erebe/wstunnel/blob/main/src/tunnel/client/client.rs#L127C54-L127C56 but I haven't quite figured out how to fix it yet, because of the spawn. But unlike the leak (#433), I'm fairly confident I can figure it out.

This task would likely be easier if wstunnel exported a "create a single tunnel" API. We create multiple wstunnel's with a single tunnel in each instantiation. Would you like a MR that created such an API?

I'm again willing to do the work here. And I am more confident on doing so than I was with the socket leak! However, some guidance on how you'd like the API to look would be appreciated.

Thanks.

[bryanlarsen]

diff --git a/src/tunnel/client/client.rs b/src/tunnel/client/client.rs
index ac4b107..7fd9938 100644
--- a/src/tunnel/client/client.rs
+++ b/src/tunnel/client/client.rs
@@ -119,16 +119,9 @@ impl<E: TokioExecutor> WsClient<E> {
                 id = request_id.to_string(),
                 remote = format!("{}:{}", remote_addr.host, remote_addr.port)
             );
-            let client = self.clone();
-            let tunnel = async move {
-                let _ = client
-                    .connect_to_server(request_id, &remote_addr, cnx_stream)
-                    .await
-                    .map_err(|err| error!("{:?}", err));
-            }
-            .instrument(span);
-
-            self.executor.spawn(tunnel);
+            self.connect_to_server(request_id, &remote_addr, cnx_stream)
+                .await
+                .context("run_tunnel")?;
         }
 
         Ok(())

Appears to work for me. I assume there's a reason for the spawn that's not exhibiting in my environment? executor.spawn returns an AbortHandle, not a JoinHandle, so we don't have access to the results of the closure. I can set up a channel so that we can return the response of connect_to_server from run_tunnel, but it'd be nice if there was a simpler way.

[erebe]

I won't have time to look into this until next week, but regarding your change

+            self.connect_to_server(request_id, &remote_addr, cnx_stream)
+                .await
+                .context("run_tunnel")?;

Not being able to connect to the server may be transient, by adding ? you are exiting the tunnel loop, which is going to destroy the tunnel.

That changes the behavior, before the tunnel was always active, even if some call fails due to transient issues with the server (i,e: changing wifi hotspot), now it aborts.
also, you can run only one tunnel at a time, because the tunnel is not executed/spawn in a dedicated task

[bryanlarsen]

Yes, this is the general conflict between a library and a command line utility.

In our case, if the user changes wifi hotspot, we can often switch the users wireguard endpoint to a non-wstunnel endpoint because only some hot spots refuse to transit bare wireguard. And if the tunnel aborts and we start a new tunnel and switch the endpoint on wireguard so the user's connection doesn't drop even though the wstunnel drops. That's a luxury most other users of wstunnel won't have though!

I was hoping the retry loop could be moved into wstunnel-cli so that we can have our cake and eat it too, but I understand your point about wanting to leave up the internal sockets so TCP connections over the tunnel don't drop even while external connections reconnect.

My suggestion for an API:

• the Client struct should describe a single connection. wstunnel-cli is then responsible for creating multiple connections.
• use a channel to return non-fatal errors. wstunnel-cli can send those to stderr, I can abort the connection, other users can ignore or otherwise choose differently.