Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove special treatment of "host" header #988

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

mseri
Copy link
Collaborator

@mseri mseri commented Jul 6, 2023

As correctly pointed out by @anuragsoni, the RFC says that order of header fields isn't significant within a well-behaved server.

The order in which header fields with differing field names are
received is not significant. However, it is good practice to send
header fields that contain control data first, such as Host on
requests and Date on responses, so that implementations can decide
when not to handle a message as early as possible. A server MUST NOT
apply a request to the target resource until the entire request

Since this introduces unnecessary complexity and unnecessary extra functions in the http package header module, I am removing the special treatment in this PR.

Users that want to enforce the ordering of the headers can do it with the same type of overhead directly in their code and can take 0d252ec as inspiration. Using Header.to_list and of_list in their code it is also safer than what we do here, in case the implementation details of the header module change.

mseri added 2 commits July 6, 2023 09:35
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri
Copy link
Collaborator Author

mseri commented Jul 6, 2023

Ping @anuragsoni @avsm

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri
Copy link
Collaborator Author

mseri commented Jul 6, 2023

In this case, we are favouring simplicity of the implementation over best practices (see also the relevant issue: #934)

@mseri
Copy link
Collaborator Author

mseri commented Aug 8, 2023

Moreover users that want to enforce Host as the first header can always add it as first header in the Headers parameter.

@mseri mseri merged commit 6b5202e into mirage:master Aug 8, 2023
17 of 19 checks passed
@mseri mseri deleted the remove-header-host-special branch August 8, 2023 15:25
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…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~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…p-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~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
…p-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~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant