Update for Eio 0.3 #886

talex5 · 2022-06-29T11:29:51Z

Use accept_fork instead of deprecated accept_sub.
Remove unnecessary calls to Flow.close; it will get closed anyway when the connection handler returns.

Doing the close manually triggers a bug in eio 0.3: ocaml-multicore/eio#244 - I didn't notice because I was testing a version of cohttp where I already removed the extra closes.

This PR is just the minimal changes to fix the build. I'll submit another PR on top of this that makes use of the new features soon.

talex5 · 2022-06-29T11:36:47Z

BTW, ocaml-ci hasn't been running on this repository for some time due to a malformed .ocamlformat file. If ocamlformat isn't being used, we could delete this file to get CI working again.

- Use `accept_fork` instead of deprecated `accept_sub`. - Remove unnecessary calls to `Flow.close`; it will get closed anyway when the connection handler returns, and doing it manually triggers a bug in eio 0.3: ocaml-multicore/eio#244

mseri · 2022-07-04T07:31:09Z

cohttp-eio/src/server.ml

@@ -95,17 +95,13 @@ let rec handle_request reader writer flow handler =
      if not (is_custom body) then Writer.wakeup writer;
      if Http.Request.is_keep_alive request then
        handle_request reader writer flow handler
-      else Eio.Flow.close flow
-  | (exception End_of_file) | (exception Eio.Net.Connection_reset _) ->
-      Eio.Flow.close flow


Is this always the case? Is there a risk that this changes in the future or that having an explict close would make it close earlier?

A priori, I would feel safer to always call the close, eventually with a catch to drop the relevant double close errors

Yes, it will always close. Eio ensures that FDs cannot be leaked. And it would be an error to close the connection and then continue trying to handle it. In the git version of Eio trying to close it manually here is a compile-time error (and in Eio 0.3, due to a bug, closing it manually results in an error being reported).

mseri · 2022-07-04T08:19:55Z

cohttp-eio/src/server.ml

@@ -114,7 +110,8 @@ let run_domain ssock handler =
  in
  Switch.run (fun sw ->
      while true do
-        Eio.Net.accept_sub ~sw ssock ~on_error (fun ~sw flow _addr ->
+        Eio.Net.accept_fork ~sw ssock ~on_error (fun flow _addr ->
+            Eio.Switch.run @@ fun sw ->


One last question. What is the reason one needs to nest the two Switch.run? Is it because there is the fork involved and then we want to manage a separate set of fibers?

It's because the writer fiber should be tied to the lifetime of this connection, not the lifetime of the server (if the handler function crashes, we want to cancel the writer fiber).

Note that this switch goes away in the next commit anyway (#887).

mseri · 2022-07-04T15:16:59Z

The macos failure has nothing to do with this. I opened a separate issue for it

…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)

talex5 force-pushed the eio-0.3 branch from b48d905 to 02f45d2 Compare June 29, 2022 11:34

talex5 force-pushed the eio-0.3 branch 2 times, most recently from 2de49c0 to 2fc786b Compare June 29, 2022 12:00

talex5 mentioned this pull request Jun 29, 2022

cohttp-eio: improve server API #887

Merged

mseri mentioned this pull request Jul 3, 2022

refactor: share bytebuffer between lwt/async #900

Merged

Update for Eio 0.3

2ce6b53

- Use `accept_fork` instead of deprecated `accept_sub`. - Remove unnecessary calls to `Flow.close`; it will get closed anyway when the connection handler returns, and doing it manually triggers a bug in eio 0.3: ocaml-multicore/eio#244

mseri reviewed Jul 4, 2022

View reviewed changes

talex5 force-pushed the eio-0.3 branch from 2fc786b to 2ce6b53 Compare July 4, 2022 08:16

mseri reviewed Jul 4, 2022

View reviewed changes

mseri merged commit 4032686 into mirage:master Jul 4, 2022

talex5 deleted the eio-0.3 branch July 4, 2022 15:18

mseri mentioned this pull request Oct 24, 2022

[new release] http, cohttp, cohttp-top, cohttp-server-lwt-unix, cohttp-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha0) ocaml/opam-repository#22353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for Eio 0.3 #886

Update for Eio 0.3 #886

talex5 commented Jun 29, 2022

talex5 commented Jun 29, 2022

mseri Jul 4, 2022

talex5 Jul 4, 2022

mseri Jul 4, 2022

talex5 Jul 4, 2022

mseri commented Jul 4, 2022

Update for Eio 0.3 #886

Update for Eio 0.3 #886

Conversation

talex5 commented Jun 29, 2022

talex5 commented Jun 29, 2022

mseri Jul 4, 2022

Choose a reason for hiding this comment

talex5 Jul 4, 2022

Choose a reason for hiding this comment

mseri Jul 4, 2022

Choose a reason for hiding this comment

talex5 Jul 4, 2022

Choose a reason for hiding this comment

mseri commented Jul 4, 2022