Eio.Switch.run hangs cleaning up resources of failed websocket connection

[copy]

Using the wscat example modified to resolve the promise on error, and with some extra logging:

diff --git a/examples/eio/wscat.ml b/examples/eio/wscat.ml
index ad6b645..573872e 100644
--- a/examples/eio/wscat.ml
+++ b/examples/eio/wscat.ml
@@ -30,9 +30,10 @@ let websocket_handler env ~sw u wsd =
   ; eof
   }
 
-let error_handler = function
+let error_handler u = function
   | `Handshake_failure (rsp, _body) ->
-    Format.eprintf "Handshake failure: %a\n%!" Httpun.Response.pp_hum rsp
+    Format.eprintf "Handshake failure: %a\n%!" Httpun.Response.pp_hum rsp;
+    Promise.resolve u ();
   | _ -> assert false
 
 let () =
@@ -78,7 +79,11 @@ let () =
         ~host
         ~port
         ~resource
-        ~error_handler
+        ~error_handler:(error_handler u)
         ~websocket_handler:(websocket_handler env ~sw u)
       in
-      Promise.await p))
+      Promise.await p;
+      Format.eprintf "After Promise.await\n%!"
+    );
+    Format.eprintf "After Switch.run\n%!"
+  )

With a random server that doesn't accept websocket connections, the promise resolves, but the Eio.Switch.run never returns (presumable because there are hanging fibers):

% dune exec ./examples/eio/wscat.exe 136.244.81.69
Handshake failure: ((version "HTTP/1.1") (status 404) (reason "Not Found") (headers 
(("Date" "Tue, 10 Sep 2024 23:04:03 GMT")
("Content-Type" "text/html; charset=utf-8")("Content-Length" "146")
("Connection" "keep-alive"))))
After Promise.await

Note that websocket_handler is never called, so I don't think it's the input reading loop.

[anmonteiro]

You must call Httpun_eio_ws.Client.shutdown.

• change the promise to a (unit, unit) result Promise.t
• add

      (match Promise.await p with
      | Error _ -> Httpun_ws_eio.Client.shutdown client |> Promise.await;
      | Ok () -> ());

the client now shuts down successfully. Alternatively, the error handler could take a ~(sw: Eio.Switch.t) argument and call Switch.fail sw exn.

[copy]

You must call Httpun_eio_ws.Client.shutdown.

Is there a technical reason for that? Once the error handler has been called, the connection is dead, so I would assume that it's closed automatically. At the very least, I would suggest documenting this requirement.

[anmonteiro]

I suppose we can do both: there are errors that aren't recoverable, and we could shutdown the runtime immediately; we should likely also document this limitation if we can't make it work in the general case.

The reasoning is two-fold, I think:

1. httpun-ws is not yet that battle-tested and further improvements are constantly being worked on (thanks for reporting issues as you're finding them, btw)
2. this is a rather low-level library that other packages such as dream or piaf build upon