vbmithr / ocaml-websocket

Websocket library for OCaml
ISC License
161 stars 44 forks source link

Exception when client closes connection. Is this the expected behavior for async? #117

Open eugecm opened 4 years ago

eugecm commented 4 years ago

Please bear with me as I'm new to OCaml and the whole ecosystem. Currently using websocket_async.

The current implementation seems to raise an exception whenever a client disconnects. This can be easily reproduced with wscat-async on master as follows:

A server:

> ./wscat.exe -s ws://localhost:9999

Client

> ./wscat.exe ws://localhost:9999/ws

Then press Control-C on the client.

The output of the server:

2020-04-13 22:02:12.066187+01:00 Error ("Exception raised to [Monitor.try_with] that already returned.""This error was captured by a default handler in [Async.Log]."(exn(monitor.ml.Error End_of_file("Raised at file \"core/websocket.ml\", line 268, characters 14-31""Called from file \"src/deferred0.ml\", line 54, characters 64-69""Called from file \"src/job_queue.ml\", line 167, characters 6-47""Caught by monitor Monitor.protect"))))

Which points to https://github.com/vbmithr/ocaml-websocket/blob/49e991b7fe25fa20340a2acd8db09b33f865673f/core/websocket.ml#L268

I guess it can make sense to raise an exception there, however (and this is what caught my attention), the async implementation also propagates the exception back to the caller as seen in: https://github.com/vbmithr/ocaml-websocket/blob/49e991b7fe25fa20340a2acd8db09b33f865673f/async/websocket_async.ml#L287-L298 (Monitor.protect propagates)

The server function returns an 'a Async.Deferred.Or_error.t but according to the documentation for Async_kernel__.Deferred_or_error the idiom is to never raise:

The mental model for a function returning an 'a Deferred.Orerror.t is that the function never raises. All error cases are caught and expressed as an Error result. This module preserves that property.

Should protect be changed for a try_with_or_error?

SeedyROM commented 11 months ago

No answer in 3 years huh?

vbmithr commented 11 months ago

I don't have time to maintain it anymore. Maybe check out https://github.com/janestreet/async_websocket for websocket server with async.