Expose the new on_exn hook in cohttp_lwt_unix?

[apatil]

Cohttp_lwt_unix recently added an on_exn hook: https://github.com/mirage/ocaml-cohttp/pull/518/files which might be a useful addition to Opium. The use case I have in mind is long polling, where the Opium_rock.Service is a long-running Lwt thread. If the client closes the connection, that thread should be cancelled.

I could probably manage a PR, but it would change some module signatures so I thought I would check with you first. Would you be open to exposing on_exn, and if so where?

[rgrinberg]

Yes, this functionality has to be exposed somehow but I'm not sure a hook is such a good idea. Opium's architecture dictates function composition as the cornerstone so this random hook is neither here nor there. Ideally, if it was possible to rethrow this exception in the user's handler I'd prefer that.

But in your particular case (long polling), you should just be able to catch the exception right in your handler from what I understand. That on_exn hook should be mostly useful for catching exceptions that occured before the request was created.

[apatil]

I agree that having an exception in the handler thread would be nicer than a hook, but I don't think it works that way currently. Try running test_longpoll.ml below with

ocamlfind ocamlc -package opium.unix -linkpkg test_longpoll.ml -o test_longpoll.byte && ./test_longpoll.byte

then curl localhost:8081 but ctrl-c the curl process within 5s. The failure handler never fires, Opium eventually tries to send the response as if the connection were still alive.

(* test_longpoll.ml *)

open Opium.Std
open Lwt.Infix

let processing_time = 5.0

let handler req =
  print_endline "Beginning handler";
  let handler_thread = Lwt_unix.sleep processing_time in
  let success_handler = fun () ->
    print_endline "Sending success response";
    respond' @@ `String "Hello\n"
  in
  let failure_handler = fun exn ->
    print_endline "There was a failure";
    Lwt.fail_with "There was a failure"
  in
  Lwt.try_bind (fun () -> handler_thread) success_handler failure_handler

let _ =
  App.empty
  |> get "/" handler
  |> Opium_app.port 8081
  |> App.run_command

[rgrinberg]

On 01/16, Anand Patil wrote:

I agree that having an exception in the handler thread would be nicer than a hook, but I don't think it works that way currently. Try running test_longpoll.ml below with

I see, thanks. I'd like to understand why this isn't the case first though.

[mattjbray]

I have a similar use-case, except my handler returns a streaming body. I'd like to cancel the Lwt thread that is pushing to the Lwt stream if the client goes away.

As far as I know, the only way to tell if the client has disconnected is to try to write some data to it, and catch the resulting exception. In Cohttp, this happens after the user's callback has returned, which is why the exception cannot be caught in the callback.

I think, in the long-polling case @avsm mentioned, it's not possible to tell that a client has disconnected until the long-running Lwt thread finishes and Cohttp attempts to write the data to the client.

In the streaming case, we can tell that the client has gone away as soon as Cohttp flushes the next available chunk of data on the Lwt stream to the client. We can catch the exception by providing on_exn (or by setting Lwt.async_exception_hook, which is the default if on_exn is not provided).

The problem now is that there is no way to determine which request/connection caused the exception when we catch it in on_exn (or Lwt.async_exception_hook). If we could, then the user could keep in memory a Map of Lwt threads, indexed by their request/connection ID. In the on_exn hook, the user could look up the Lwt thread corresponding to this request, and cancel it.

Cohttp also provides a conn_closed hook. In this hook we do have access to the request/connection ID that was closed. However, currently this is not called if Cohttp encounters an exception while writing the response data to the client.

In summary I think the following changes would enable this use case:

1. In Cohttp, catch exceptions while writing to the client to ensure that conn_closed is always called. This could be done using Lwt.finalize in cohttp_lwt.ml, as shown in this commit.

2. In Opium, expose the conn_closed hook, and expose the Cohttp.Connection.t callback argument to handlers.

   However, as you say this is not Opium's style, so in this commit I propose an alternative:

   • Add a field conn_closed : (unit -> unit Lwt.t) option to Opium_rock.Response.t. Users can set this using Opium_rock.Response.create.
   • Opium tracks the user's conn_closed hooks, and executes the hooks in Cohttp's global conn_closed hook.

   The commit also contains a streaming_response.ml example, which can be run after pinning the modified Cohttp:

   opam pin add cohttp 'git://github.com/mattjbray/ocaml-cohttp#ensure-conn-closed'
   make && ./examples/streaming_response --verbose

What do you think of this approach? If you think it's viable, I can prepare some pull requests.

[mattjbray]

I've submitted a PR for the conn_closed change in Cohttp: https://github.com/mirage/ocaml-cohttp/pull/528

To answer my own question, my implementation for part 2 above feels like a hack. I'd love to be told that there is a better way to do this.

In the Cohttp PR I've suggested supporting a per-connection cleanup hook that the user can provide from the callback. @rgrinberg, if this was added to Cohttp would you be open to exposing it in Opium?

[rgrinberg]

In the Cohttp PR I've suggested supporting a per-connection cleanup hook that the user can provide from the callback. @rgrinberg, if this was added to Cohttp would you be open to exposing it in Opium?

I'm open to it but I'd like to understand why it's necessary at all. It doesn't seem like exposing these low level details is done in similar libraries, so I want to be convinced before committing.

[mattjbray]

Thanks for taking the time to consider this; I totally understand. Let's continue discussion in https://github.com/mirage/ocaml-cohttp/issues/529.

[anuragsoni]

Closing this as the current master branch has switched to httpaf