Add expert mode for cohttp-lwt #647

andreas · 2018-11-05T20:03:04Z

This PR adds "expert mode" to cohttp-lwt similar to what exists for cohttp-async (#488).

My primary use case is for websocket support like #488. The changes also relate to other issues regarding streaming though (#534, #529), and this approach could be used to solve those use cases. An Expert-response does currently not allow omitting a body (a decision inherited from #488), but if that was permitted, then it could be used to for writing/streaming bigarrays efficiently (mentioned here).

I've rewritten the response/response handling, such that it no longer uses Lwt_stream. Though the abstraction of mapping a stream of requests to a stream of responses sounds elegant, I think it's actually simpler without it. In particular the use of refs (early_close) and mutexes (read_m) is no longer required. I'm happy to receive feedback on the choice though.

I've not added any tests, but I'd be happy to if the overall approach is approved.

/cc @mattjbray

avsm

Approach looks great, and would be of course good to support websockets. I left some comments about the approach in the PR comments.

cohttp-lwt/src/s.ml

avsm · 2018-11-08T11:31:15Z

This approach and interface lgtm! Some tests to validate it stays working for websocket would be great before merging if possible.

mattjbray · 2018-11-08T12:34:29Z

This solves my streaming use case (catch exceptions while writing the body) so 👍 from me. This will close #534 and #529.

andreas · 2018-11-08T17:00:00Z

Some tests to validate it stays working for websocket would be great before merging if possible.

Will work on adding tests 👍

This solves my streaming use case (catch exceptions while writing the body)

The current formulation of `Expert always writes an empty body for the initial response (source -- this is the behavior in Cohttp_async from #488). When switching protocols, this is fine. However, the streaming use case is not supported with this behavior. We could either choose to not write an empty body for an `Expertresponse (change of semantics), or expand the variant to have an optional body:

type response_action =
  [ `Expert of Cohttp.Response.t
+               * Body.t option
               * (IO.ic
                  -> IO.oc
                  -> unit Lwt.t)
  | `Response of Cohttp.Response.t * Body.t ]

In general, I would urge that we keep the type response_action and its semantics in sync between Cohttp_lwt and Cohttp_async. This is essential for being able to write a websocket library that works with both without requiring adapters left and right. Changing `Expert in Cohttp_async would be a breaking change though 😢

I'm a little concerned about this PR snowballing into a lot more changes though, as I'd like to see this PR merged...

avsm · 2018-11-08T17:06:52Z

One option is to be explicit with the purpose of the response_action, and have:

type response_action =
  [ 
  `Switch_protocol of Cohttp.Response.t              
               * (IO.ic
                  -> IO.oc
                  -> unit Lwt.t)
 | `Stream of Cohttp.Response.t * Body.t 
              * (IO.ic
                  -> IO.oc
                  -> unit Lwt.t)
  | `Response of Cohttp.Response.t * Body.t ]

Although I'm a little confused as to the purpose of the empty body write in the Async case. What's that actually sending out to the wire -- an empty HTTP chunked encoding ?

andreas · 2018-11-09T15:23:35Z

Although I'm a little confused as to the purpose of the empty body write in the Async case. What's that actually sending out to the wire -- an empty HTTP chunked encoding ?

I'm not sure why writing the empty body was part of #488 in the first place. It appears the main goal of that PR was to enable switching protocols, which can work despite writing the empty body when encoding is set to Transfer.Unknown (see below).

What's written on the wire depends on the encoding set on the response:

For Transfer.Chunked, the empty body is written out as 0\r\n\r\n. This prohibits writing further to the body.

`Expert (
  Cohttp.Response.make ~encoding:Transfer.Chunked (),
  fun ic oc -> (* client considers body finished at this point *)
)

HTTP/1.1 200 OK
Transfer-Encoding: chunked

0\r\n
\r\n

For Transfer.Fixed, nothing is written for the body. So this works for writing a fixed-size response in the IO handler function, but not for streaming responses.

`Expert (
  Cohttp.Response.make ~encoding:Transfer.(Fixed 3L) (),
  fun ic oc -> Lwt_io.write oc "foo"
)

HTTP/1.1 200 OK
Content-Length: 3

foo

For Transfer.Unknown, nothing is written for the body. This is good for creating the 101 Switching Protocols response, which I guess is why this was not discussed in Expose [Reader.t] and [Writer.t] via [`Switching_protocols] for websockets #488.

let headers = Header.of_list [
  "Upgrade", "websocket";
  "Connection", "Upgrade"
] in
`Expert (
  Cohttp.Response.make ~status:`Switching_protocols ~headers ~encoding:Transfer.Unknown (),
  fun ic oc -> (* websocket stuff *)
)

HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade

You can also abuse this to set a Transfer-Encoding header and then reply with a chunked response:

let headers = Header.of_list [
  "Transfer-Encoding", "chunked"
] in
`Expert (
  Cohttp.Response.make ~headers ~encoding:Transfer.Unknown (),
  fun ic oc ->
    Lwt_io.write oc "3\r\nfoo\r\n" >>= fun () ->
    Lwt_io.write oc "3\r\nbar\r\n" >>= fun () ->
    Lwt_io.write oc "0\r\n\r\n"
)

HTTP/1.1 200 OK
Transfer-Encoding: chunked

3\r\n
foo\r\n

3\r\n
bar\r\n

0\r\n
\r\n

This feel like a hack though.

One option is to be explicit with the purpose of the response_action, and have:

type response_action =
  [ 
  `Switch_protocol of Cohttp.Response.t              
               * (IO.ic
                  -> IO.oc
                  -> unit Lwt.t)
 | `Stream of Cohttp.Response.t * Body.t 
              * (IO.ic
                  -> IO.oc
                  -> unit Lwt.t)
  | `Response of Cohttp.Response.t * Body.t ]

Given the above, I have a hard time coming up with a use case for writing providing both Body.t and an IO function IO.ic -> IO.oc -> unit io: why would you both write out a body, which will subsequently be considered finished by the client, and have a function for writing further to this body?

Unless I'm missing something, I would suggest to change the semantics for `Expert to not write an empty response body.

andreas · 2018-11-13T21:05:49Z

Anything I can do to help this move forward? If we can agree on the approach, I'd be happy to continue with implementation and tests (both Async and Lwt).

mseri · 2018-11-13T21:21:09Z

I think that changing the semantics of `Expert makes more sense and is cleaner and easier to reason about. We could ping @avsm to have also his opinion

avsm · 2018-11-14T12:44:07Z

Unless I'm missing something, I would suggest to change the semantics for `Expert to not write an empty response body.

Thank you for the comprehensive analysis! I agree with @andreas and @mseri that changing the semantics of Expert in makes more sense. We can merge and cut a major version bump with this change as soon as the PR is ready.

andreas · 2018-11-14T20:05:37Z

Great, thanks! I'll proceed with the implementation and tests.

avsm · 2018-11-19T17:05:03Z

@andreas when ready, if you merge with the master branch the Travis tests should now be green as well

andreas

I've updated the code now and added a basic test for both Lwt and Async.

cohttp-async/src/server.ml

cohttp-lwt-unix/src/io.ml

cohttp-lwt/src/server.ml

cohttp-lwt/src/s.ml

avsm

Fairly minor review comments from me; I'm happy to merge as soon as you're happy with the PR @andreas!

cohttp-async/src/server.mli

cohttp-async/test/test_async_integration.ml

cohttp-lwt-unix/src/io.ml

avsm · 2018-11-20T09:31:35Z

cohttp-lwt/src/server.ml

+         Lwt.catch
+           (fun () -> callback conn req body)
+           (fun exn ->
+             Format.eprintf "Error handling %a: %s\n%!" Request.pp_hum req (Printexc.to_string exn);


Should this use Logs instead of directly printing to stderr?

Logs is currently not a dependency of cohttp-lwt, but I can add it if you want?

Yes I think that would be a good idea - we need to eliminate printf errors from embeddable libraries

avsm · 2018-11-20T23:12:35Z

When ready to merge, if you could push a CHANGES entry that describes the backwards compat breaking change that would be helpful. I can roll several PRs into a major version bump.

andreas · 2018-11-21T14:49:51Z

Replaced Format.eprintf with Log.err in Cohttp_lwt.Server
Added an entry CHANGES.md

avsm · 2018-11-21T15:47:30Z

Thanks for the contribution! I'll prepare a release after a bit more testing -- in the meanwhile, ensuring that the master branch of cohttp works for your websocket/other libraries that need this feature would be most appreciated.

andreas · 2018-11-21T20:28:45Z

Yay, thanks 😀

in the meanwhile, ensuring that the master branch of cohttp works for your websocket/other libraries that need this feature would be most appreciated.

I have already developed some features against this locally -- I can share the source if it's of interest 😄

avsm · 2018-11-22T09:23:10Z

That would be great! Having more examples to point to in the README is probably one of cohttp's bigger user requests :-)

mseri · 2018-11-22T09:29:10Z

It would be nice to be able to add the README as an Introduction page in the odoc/ocamldoc generated documentation. The little tool that broke github (md2mld) was made on purpose to try and do it on ocaml-rpc. Do you know a better way?

avsm · 2018-11-22T09:42:09Z

md2mld seems just fine to me -- especially if it had a dune rule that just worked :-)

The other place where the README shows up is in the opam description field. Perhaps we should break up the README contents into a doc/ directly and assemble it via dune promotion rules into the opam/README/mld files. I did a crude version of that in the duniverse tool.

@andreas

…ohttp-top, cohttp-async and cohttp-mirage (2.0.0) CHANGES: Compatibility breaking interface changes: Async: Expert response action no longer writes empty HTTP body (mirage/ocaml-cohttp#647 by @andreas) In cohttp.0.99, a number of subpackages were turned into explicit opam packages to simplify dependency management. To aid migration, some compatability shims were left in place so that the old findlib names would continue to work. They have now been removed as of this release. If you were still using them, then please rename them as follows in your dune or ocamlbuild files: - `cohttp.lwt-core` -> `cohttp-lwt` - `cohttp.lwt` -> `cohttp-lwt-unix` - `cohttp.js` -> `cohttp-lwt-jsoo` - `cohttp.async` -> `cohttp-async` - `cohttp.top` -> `cohttp-top` Other changes and bugfixes: * Lwt, Mirage: Add log warnings for uncaught exceptions (mirage/ocaml-cohttp#592 by @ansiwen) * Log invalid client input and do not catch out of memory exceptions (mirage/ocaml-cohttp#652 @hannesm) * Port opam files to opam2 and add local synopsis and descriptions. * Lwt: Add Expert response action for servers (mirage/ocaml-cohttp#647 by @andreas) * Use the namespaced `js_of_ocaml` interfaces from 3.3.0 onwards (mirage/ocaml-cohttp#654 @avsm) * Use Base64 3.1.0 interfaces (mirage/ocaml-cohttp#655 @avsm) * Clean up redundant conflicts in the `opam` files (@avsm)

@andreas

…ohttp-top, cohttp-async and cohttp-mirage (2.0.0) (#13388) CHANGES: Compatibility breaking interface changes: Async: Expert response action no longer writes empty HTTP body (mirage/ocaml-cohttp#647 by @andreas) In cohttp.0.99, a number of subpackages were turned into explicit opam packages to simplify dependency management. To aid migration, some compatability shims were left in place so that the old findlib names would continue to work. They have now been removed as of this release. If you were still using them, then please rename them as follows in your dune or ocamlbuild files: - `cohttp.lwt-core` -> `cohttp-lwt` - `cohttp.lwt` -> `cohttp-lwt-unix` - `cohttp.js` -> `cohttp-lwt-jsoo` - `cohttp.async` -> `cohttp-async` - `cohttp.top` -> `cohttp-top` Other changes and bugfixes: * Lwt, Mirage: Add log warnings for uncaught exceptions (mirage/ocaml-cohttp#592 by @ansiwen) * Log invalid client input and do not catch out of memory exceptions (mirage/ocaml-cohttp#652 @hannesm) * Port opam files to opam2 and add local synopsis and descriptions. * Lwt: Add Expert response action for servers (mirage/ocaml-cohttp#647 by @andreas) * Use the namespaced `js_of_ocaml` interfaces from 3.3.0 onwards (mirage/ocaml-cohttp#654 @avsm) * Use Base64 3.1.0 interfaces (mirage/ocaml-cohttp#655 @avsm) * Clean up redundant conflicts in the `opam` files (@avsm)

@andreas

…ohttp-top, cohttp-async and cohttp-mirage (2.0.0) (#13388) CHANGES: Compatibility breaking interface changes: Async: Expert response action no longer writes empty HTTP body (mirage/ocaml-cohttp#647 by @andreas) In cohttp.0.99, a number of subpackages were turned into explicit opam packages to simplify dependency management. To aid migration, some compatability shims were left in place so that the old findlib names would continue to work. They have now been removed as of this release. If you were still using them, then please rename them as follows in your dune or ocamlbuild files: - `cohttp.lwt-core` -> `cohttp-lwt` - `cohttp.lwt` -> `cohttp-lwt-unix` - `cohttp.js` -> `cohttp-lwt-jsoo` - `cohttp.async` -> `cohttp-async` - `cohttp.top` -> `cohttp-top` Other changes and bugfixes: * Lwt, Mirage: Add log warnings for uncaught exceptions (mirage/ocaml-cohttp#592 by @ansiwen) * Log invalid client input and do not catch out of memory exceptions (mirage/ocaml-cohttp#652 @hannesm) * Port opam files to opam2 and add local synopsis and descriptions. * Lwt: Add Expert response action for servers (mirage/ocaml-cohttp#647 by @andreas) * Use the namespaced `js_of_ocaml` interfaces from 3.3.0 onwards (mirage/ocaml-cohttp#654 @avsm) * Use Base64 3.1.0 interfaces (mirage/ocaml-cohttp#655 @avsm) * Clean up redundant conflicts in the `opam` files (@avsm)

andreas mentioned this pull request Nov 5, 2018

New package: graphql-cohttp andreas/ocaml-graphql-server#126

Merged

avsm reviewed Nov 6, 2018

View reviewed changes

cohttp-lwt/src/s.ml Show resolved Hide resolved

cohttp-lwt/src/s.ml Outdated Show resolved Hide resolved

andreas force-pushed the lwt-expert-mode branch from 7b6c52e to 5cb87e3 Compare November 7, 2018 20:46

avsm mentioned this pull request Nov 14, 2018

Async: use Conduit V2 API #648

Merged

andreas added 2 commits November 19, 2018 20:32

Add expert mode for cohttp-lwt

899886b

No empty body for Async expert response

aa0f33d

andreas force-pushed the lwt-expert-mode branch from 5cb87e3 to b3d8e9b Compare November 19, 2018 19:34

andreas commented Nov 19, 2018

View reviewed changes

cohttp-async/src/server.ml Show resolved Hide resolved

cohttp-lwt-unix/src/io.ml Show resolved Hide resolved

cohttp-lwt/src/server.ml Show resolved Hide resolved

mseri reviewed Nov 19, 2018

View reviewed changes

cohttp-lwt/src/s.ml Outdated Show resolved Hide resolved

avsm reviewed Nov 20, 2018

View reviewed changes

andreas added 3 commits November 20, 2018 15:45

Handle Channel_closed in IO for lwt-unix

fee808a

Tests for expert responses

9cdaddd

Add doc string for response_action

3e039f5

andreas force-pushed the lwt-expert-mode branch from 5bfc2ca to 3e039f5 Compare November 20, 2018 14:45

andreas added 2 commits November 21, 2018 15:48

Use Logs for errors in cohttp-lwt

2a4dd41

Update CHANGES

52f76af

avsm merged commit 9f45a10 into mirage:master Nov 21, 2018

andreas deleted the lwt-expert-mode branch November 21, 2018 20:27

This was referenced Dec 13, 2018

First stab at websockets andreas/ocaml-graphql-server#133

Merged

Usage with Mirage vbmithr/ocaml-websocket#105

Open

avsm mentioned this pull request Feb 4, 2019

[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, cohttp-top, cohttp-async and cohttp-mirage (2.0.0) ocaml/opam-repository#13388

Merged

hongchangwu mentioned this pull request Feb 12, 2019

Websocket_cohttp_lwt.upgrade_connection doesn't work with TLS connections vbmithr/ocaml-websocket#108

Closed

This was referenced Aug 11, 2019

For discussion: Cohttp_lwt_body: Add `CustomStream type #534

Closed

Lwt streaming responses and flush exceptions #529

Closed

mseri mentioned this pull request Jan 8, 2022

Use new request parser #819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add expert mode for cohttp-lwt #647

Add expert mode for cohttp-lwt #647

andreas commented Nov 5, 2018

avsm left a comment

avsm commented Nov 8, 2018

mattjbray commented Nov 8, 2018

andreas commented Nov 8, 2018

avsm commented Nov 8, 2018

andreas commented Nov 9, 2018 •

edited

andreas commented Nov 13, 2018

mseri commented Nov 13, 2018

avsm commented Nov 14, 2018

andreas commented Nov 14, 2018

avsm commented Nov 19, 2018

andreas left a comment

avsm left a comment

avsm Nov 20, 2018

andreas Nov 20, 2018

avsm Nov 20, 2018

avsm commented Nov 20, 2018

andreas commented Nov 21, 2018

avsm commented Nov 21, 2018

andreas commented Nov 21, 2018

avsm commented Nov 22, 2018

mseri commented Nov 22, 2018 •

edited

avsm commented Nov 22, 2018

Add expert mode for cohttp-lwt #647

Add expert mode for cohttp-lwt #647

Conversation

andreas commented Nov 5, 2018

avsm left a comment

Choose a reason for hiding this comment

avsm commented Nov 8, 2018

mattjbray commented Nov 8, 2018

andreas commented Nov 8, 2018

avsm commented Nov 8, 2018

andreas commented Nov 9, 2018 • edited

andreas commented Nov 13, 2018

mseri commented Nov 13, 2018

avsm commented Nov 14, 2018

andreas commented Nov 14, 2018

avsm commented Nov 19, 2018

andreas left a comment

Choose a reason for hiding this comment

avsm left a comment

Choose a reason for hiding this comment

avsm Nov 20, 2018

Choose a reason for hiding this comment

andreas Nov 20, 2018

Choose a reason for hiding this comment

avsm Nov 20, 2018

Choose a reason for hiding this comment

avsm commented Nov 20, 2018

andreas commented Nov 21, 2018

avsm commented Nov 21, 2018

andreas commented Nov 21, 2018

avsm commented Nov 22, 2018

mseri commented Nov 22, 2018 • edited

avsm commented Nov 22, 2018

andreas commented Nov 9, 2018 •

edited

mseri commented Nov 22, 2018 •

edited