http: add Request.make #878

bikallem · 2022-06-08T12:46:34Z

Add Request.make so that we don't have to adorn [@warning "-3"] when
creating Request.t.

http/src/http.ml

mseri · 2022-06-08T20:18:32Z

The CI seems to be stuck right now, anyways it will fail badly because the javascript tests are completely broken. One failure, with conf-npm seems to be due to incompatible versions of node and npm, the other is about not finding the library for pretend xmlrpc. I thin it may make sense to disable them for now. I will do it in a separate PR later on, after merging eio

http/src/http.ml

bikallem · 2022-06-09T11:25:23Z

I followed mainly Response.make for the default value of encoding. However, shouldn't the default value of encoding be Transfer.Unknown rather than Transfer.Chunked for both request and response? However, that would be for another PR.

mseri · 2022-06-09T12:44:51Z

I don't know. In cohttp we actually use Transfer.Fixed Int64.zero as default. For the response Chunked was a safe option, not knowing the body. I think we should stick to backward compatibility in this case (encoding will be removed in any case soon)

http/src/http.ml

http/src/http.mli

mseri · 2022-06-09T13:04:12Z

I am going to make one more change and push over it. Then let's see also what @rgrinberg thinks about it

bikallem · 2022-06-10T22:02:19Z

@rgrinberg is this PR in a state to be merged?

mseri · 2022-06-10T22:17:20Z

We have discussed this in the afternoon. I am making some further changes before it can be merged, essentially we will drop encoding from the constructor, document its behaviour and update cohttp to use it.

cohttp/src/request.ml

mseri · 2022-06-13T09:55:36Z

For some reason that I don't really understand, the cohttp-lwt-unix tests seem to deadlock... or at least they get stuck forever. Any clue?

@warning

Add `Request.make` so that we don't have to adorn `[@warning "-3"]` when creating `Request.t`.

When doing this update, I realized that our responses are in general not compliant with the standard since https://tools.ietf.org/html/rfc7230#section-3.3.1 A server MUST NOT send a Transfer-Encoding header field in any response with a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send a Transfer-Encoding header field in any 2xx (Successful) response to a CONNECT request (Section 4.3.6 of [RFC7231]). But we add `transfer-encoding: unknown` to those responses (I am actually surprised that 304 is not part of those). Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

In the absence of an encoding in the headers, Http.Response.Make now defaults to Unknown, which makes the system hang unable to decide how to get the body. Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri · 2022-06-14T21:06:25Z

Ok, I made it as small as possible. The cohttp functions {Request,Response}.make remain backward compatible, and the Http ones are saner and drop the encoding parameter. The code is updated in a backward-compatible way, the only thing that needed fixing was one test, since now the Response encoding defaults to Unknown so the responses need to explicitly set a encoding header if they use the dumb Http constructor (or they need to rely to the old one in Cohttp)

mseri · 2022-06-14T21:14:47Z

I'd like both your opinions on this one before merging.
I agree with @rgrinberg on having the http constructors as dumb as possible and do nothing fancy, but I could not modify the cohttp ones in a backward compatible way since it would require changing the headers and did end up in some weird corner cases (like response 304 or 1xx/204 having a Fixed 0 transfer-encoding instead of Unknown)

bikallem · 2022-06-15T15:11:46Z

LGTM.

It needs to construct responses with the correct transfer-encoding. Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

…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)

mseri reviewed Jun 8, 2022

View reviewed changes

http/src/http.ml Outdated Show resolved Hide resolved

bikallem force-pushed the request-make branch 2 times, most recently from 9609177 to 412a245 Compare June 8, 2022 18:30

mseri reviewed Jun 8, 2022

View reviewed changes

http/src/http.ml Outdated Show resolved Hide resolved

mseri reviewed Jun 8, 2022

View reviewed changes

http/src/http.ml Outdated Show resolved Hide resolved

bikallem force-pushed the request-make branch 2 times, most recently from e4c6b16 to 5a1b6a9 Compare June 9, 2022 09:04

mseri reviewed Jun 9, 2022

View reviewed changes

http/src/http.ml Show resolved Hide resolved

mseri reviewed Jun 9, 2022

View reviewed changes

http/src/http.ml Outdated Show resolved Hide resolved

mseri reviewed Jun 9, 2022

View reviewed changes

http/src/http.mli Outdated Show resolved Hide resolved

mseri force-pushed the request-make branch 2 times, most recently from d5c57b6 to 2284fa6 Compare June 9, 2022 14:59

mseri reviewed Jun 10, 2022

View reviewed changes

cohttp/src/request.ml Outdated Show resolved Hide resolved

bikallem and others added 2 commits June 14, 2022 22:53

http: add Request.make

fcff6d3

Add `Request.make` so that we don't have to adorn `[@warning "-3"]` when creating `Request.t`.

mseri force-pushed the request-make branch from f0fdbbe to 8a80b32 Compare June 14, 2022 20:53

mseri added 2 commits June 14, 2022 22:58

cohttp-lwt-jsoo: adapt to new http constructors

918dfff

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

cohttp-lwt-unix: adapt to new http constructors

e89bd22

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri force-pushed the request-make branch 2 times, most recently from de54531 to bd1aa51 Compare June 14, 2022 21:00

mseri added 2 commits June 14, 2022 23:02

cohttp-lwt-unix: fix test_sanity.exe

106ccc8

In the absence of an encoding in the headers, Http.Response.Make now defaults to Unknown, which makes the system hang unable to decide how to get the body. Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

cohttp-async: use the correct Response constructor

1aa8b1f

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

mseri force-pushed the request-make branch from bd1aa51 to 1aa8b1f Compare June 14, 2022 21:03

rgrinberg approved these changes Jun 15, 2022

View reviewed changes

mseri and others added 2 commits June 16, 2022 10:00

cohttp-async: fix test_helper

b8a188e

It needs to construct responses with the correct transfer-encoding. Signed-off-by: Marcello Seri <marcello.seri@gmail.com>

cohttp-lwt-unix: revert spurious change in test_sanity_noisy

d1b57c7

mseri merged commit 69d6826 into mirage:master Jun 16, 2022

bikallem deleted the request-make branch June 16, 2022 09:20

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

http: add Request.make #878

http: add Request.make #878

bikallem commented Jun 8, 2022

mseri commented Jun 8, 2022

bikallem commented Jun 9, 2022 •

edited

Loading

mseri commented Jun 9, 2022

mseri commented Jun 9, 2022

bikallem commented Jun 10, 2022

mseri commented Jun 10, 2022

mseri commented Jun 13, 2022

mseri commented Jun 14, 2022

mseri commented Jun 14, 2022

bikallem commented Jun 15, 2022

http: add Request.make #878

http: add Request.make #878

Conversation

bikallem commented Jun 8, 2022

mseri commented Jun 8, 2022

bikallem commented Jun 9, 2022 • edited Loading

mseri commented Jun 9, 2022

mseri commented Jun 9, 2022

bikallem commented Jun 10, 2022

mseri commented Jun 10, 2022

mseri commented Jun 13, 2022

mseri commented Jun 14, 2022

mseri commented Jun 14, 2022

bikallem commented Jun 15, 2022

bikallem commented Jun 9, 2022 •

edited

Loading