For discussion: Cohttp_lwt_body: Add `CustomStream type#534
For discussion: Cohttp_lwt_body: Add `CustomStream type#534mattjbray wants to merge 2 commits intomirage:masterfrom
Conversation
Gives the user finer control over writing the body. In particular, the user can catch exceptions raised during the write operation.
|
I think this approach is definitely the correct approach in general. Lwt_stream is quite fragile and should be deprecated altogether. The other benefits of this approach is that it can be made uniform between Async and Lwt.
We need a flushing mechanism but it's probably wrong to copy how WAI does it. There are 2 saner ways to signal flushing:
I believe WAI used to do the former until they wanted to save some allocations (for reasons that I don't think should affect us). I'm leaning towards the former as well, but I'd like to check how Snap handles this as well.
This doesn't follow our naming convention. It should be |
|
It would be nice to have support for cstruct/bigstring-like streams at some point in Cohttp to limit copying. Or Astring-style substrings. So perhaps the name could reflect that this is a stream of strings? |
|
Most flexible here would be a stream of bodies :P. But for now perhaps we can keep it simple and accept chunks that are either substrings or cstructs |
Agreed! |
The motivation for this change is to give the user more control over how and when the response body is written. In particular, this allows the user to catch exceptions raised during the write operation.
See #528, #529 (sorry for spreading this out over multiple issues).
This API is similar to how Haskell's WAI implements streaming responses.
Benefits:
on_exnhook, but there is no way for the user to know which connection caused the exception. The user can get finalizer-like functionality (since Ensure conn_closed is called if client goes away. #528) by keeping track of connection IDs and matching them up in theconn_closedhook. TheCustomStreamAPI provides a much simpler way of doing this.Cohttp_lwt_body.t, so downstream libraries (e.g. Opium, Expose the new on_exn hook in cohttp_lwt_unix? rgrinberg/opium#65) are not affected. It is not a breaking change, c.f. adding per-connectionconn_closed/on_exnhooks.Drawbacks:
Other issues:
CustomStream.flushfunction. This seemed like a more invasive change, and I haven't got my head around the mechanics of flushing in Cohttp yet, so I haven't tried this. Do we need to?is_empty,to_streamandlength.