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

Async client: do not read body if there is none #671

Merged
merged 2 commits into from
Aug 11, 2019
Merged

Conversation

emillon
Copy link
Contributor

@emillon emillon commented Jul 24, 2019

Responses with code 204 (No content) and 304 (Not modified) do not have
bodies. They can come with no Content-Length header, in which case the
transfer encoding is inferred as Transfer.Unknown. When trying to read
the nonexistent body, it would do so using Transfer_io.Unknown.read,
which attempts to read until EOF, which can cause hangs.

There is some logic in Response.has_body to determine if we should
attempt to read a body at all. This was hooked in the Lwt backend but
not in the Async one.

Closes #666

Responses with code 204 (No content) and 304 (Not modified) do not have
bodies. They can come with no Content-Length header, in which case the
transfer encoding is inferred as `Transfer.Unknown`. When trying to read
the nonexistent body, it would do so using `Transfer_io.Unknown.read`,
which attempts to read until EOF, which can cause hangs.

There is some logic in `Response.has_body` to determine if we should
attempt to read a body at all. This was hooked in the `Lwt` backend but
not in the `Async` one.

Closes mirage#666
@emillon
Copy link
Contributor Author

emillon commented Jul 24, 2019

Not sure how to write an automated test for this, but this fixes the 204 & 304 cases in #666.

@avsm
Copy link
Member

avsm commented Aug 11, 2019

This lgtm, thanks!

@avsm avsm merged commit c37f71f into mirage:master Aug 11, 2019
avsm added a commit to avsm/opam-repository that referenced this pull request Aug 18, 2019
…c, cohttp-top, cohttp-lwt-unix and cohttp (2.3.0)

CHANGES:

- use conduit-mirage instead of mirage-conduit, which was renamed
  upstream in conduit. The minimum OCaml version supported for
  conduit-mirage is now OCaml 4.07 and higher. (mirage/ocaml-cohttp#672 @avsm)
- remove deprecation warnings in OCaml 4.08.0 using stdlib-shims (mirage/ocaml-cohttp#672 @avsm)
- async: do not read body if none is present (mirage/ocaml-cohttp#671 @emillon)
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.

204 causes cohtt-async to hang
2 participants