report exceptions before closing the body

[dpatti]

In #148 we made sure the close and flush the body so that the receiver
wouldn't potentially be left waiting. This changes the order so that the
error handler is always called first, and then the body eof is sent. In
practice, user code will typically wait for the response handler to be
called, and at that point their only signal that the request is complete
is to wait for the body eof. Likewise, the signal that there was an
error and the request will not complete is the error handler. You can
imagine having some client setup code that looks like this:

let result = Ivar.create () in
let on_eof () = Ivar.fill_if_empty result (Ok ()) in
let error_handler e = Ivar.fill_if_empty result (Error e) in
(* ... *)

By making sure we fire the error handler first, the user can correctly
identify the result of the request and not accidentally mark it as
complete. Fixes #187.

[dpatti]

In #148 we made sure the close and flush the body so that the receiver wouldn't potentially be left waiting

I actually don't remember if this is true. I can revert the patch in that commit and instead simply change shutdown t to Reader.force_close t.reader and that still makes all the tests pass. So that means an alternative change here would be to not flush the body and assume that the client knows to stop waiting because the error handler was called. There is of course a third world where, once the response handler is called, we assume that the error handler will no longer be called and instead add an ~on_error to Body.schedule_read, essentially moving the callback closer to where the user might expect it.

[seliopou]

Any ideas as to how this is going to affect lwt- and async-based runtimes? I was thinking about the error handler, and it seems to me that there should be some invariant attached to it where it should only be able to do something like flip a bit somewhere to say handle this later. Specifically in the async case, it could always just call Monitor.send_exn request_monitor.

So to bring it back to the actual PR, this may have unintended consequences if work is being done in the error handler that either: 1. might affect her state machine; or 2. might raise. If we say that neither of those two things are allowed to happen, and we update the lwt and async libs accordingly, then I think this is good.

[dpatti]

1. might affect her state machine

I'm not sure what you mean by this – the httpaf state machine, or the user's state machine? Like if they try to write a response or something?

2. might raise

This one I get. We've had numerous issues where a handler raising can leave things in a bad state. I sort of think that the runtimes should wrap user callbacks and ensure httpaf isn't left in a weird state, but it's also a bit of a rabbit hole. I was going to say we could just use Fun.protect and move on to other things, or leave this for now.

I guess in general, having runtimes schedule handlers instead of invoking them synchronously seems like a good trade-off to make things easier to reason about.