-
Notifications
You must be signed in to change notification settings - Fork 174
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
Do not omit mandatory null content-length headers. #985
Conversation
df8ad8d
to
d0186f9
Compare
@anuragsoni can you have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
One note for future releases is to re-think how we track this in a future release as the encoding
field this api depends on has been deprecated and will be removed in a future release (https://github.com/mirage/ocaml-cohttp/blob/master/http/src/http.mli#L388-L389)
Hmm, that said, going through the async implementation again as a refresher, it looks like the current scenarios that are handled are:
The header module does perform a check if there's a transfer encoding header present before updating the header value so this change should be safe, but it'd be nice to either run this header setup in the individual backends based on the body types written for each backend (I think I lean towards linking the encoding type to the body as opposed to request as it is in the current cohttp version), or rethink whether the encoding type needs to be deprecated/removed. |
I also think it would be better to automatically deduce this from the body type and entirely abstract this from the users, because in the vast majority of cases they do not care. HTTP 1 is a bit messy because it mixes transport-level and application-level headers, but IMO It is also probably possible and desirable to factor all these behavior once and for all in the common |
+1
I haven't looked at the other pull-request, (and I'm not a maintainer for cohttp so take this with a grain of salt), but I found the shared signature/functor approach in cohttp a little awkward to work with when working on the new parser. This was exacerbated in the lwt backend where there was another layer added on top with the split between Personally my preference after working on cohttp and opium (which used to be built on top of cohttp), leans towards using the |
I'm not talking about functors here, just : module type CLIENT = sig
type 'a io
val call : request -> body -> (response * body) io
end
module Client_lwt : CLIENT with type 'a io = 'a Lw.t
module Client_eio : CLIENT with type 'a io = 'a The end users don't need to instantiate a functor, they can use the values directly. Actually they needn't even know there is a common signature. The only aim would be indeed to help maintaining consistency, so that when switching backend the same feature set / API is guaranteed. One could also imagine writing a library that works with any cohttp backend, but that would indeed require a functor. Now here I'm not even arguing for / against this, I just want to emulate the current API word-for-word to ease possible adoption of that MR, without bundling API design choice in it; that can come in a second time. These discussion probably belong over there, for that matter :) |
LGTM. can you rebase? |
d5f776c
to
cdeed90
Compare
Content-Length, even if null, is mandatory for unchunked request which method may allow a body.
cdeed90
to
e582f5d
Compare
Thanks! |
…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)
…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)
…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)
The code currently reuses the code that determines whether a body should be read to determine whether a
Content-Length
header should be sent. However, for unchunked bodies of size 0, there is no body to read, butContent-Length: 0
is still mandatory for methods which may allow a body.