http4s/http4s-okhttp-client

Body of request can "go missing" when constructed using the apply method of Request

bastewart opened this issue · 4 comments

http4s version 0.20.0-M4.

I have created a repo to demonstrate this. I'm on gitter if anything needs an explanation and it would clutter here. https://github.com/bastewart/http4s-body-missing-test

I think this may have the same cause as http4s/http4s#2346 because when the body goes missing the request logger also does not log properly.

Possibly something is (in a non-functional way) consuming the Stream that represents the request body? Enabling the request logger for OkHttp leads to an error caused by reading the response body twice (see README of my repo) which also indicates non-functional consumption.

In a nutshell when I construct a request "manually" (apply method of Request) then the body does not get sent by the client if the implementation is Async or OkHttp. It does work for Jetty and Blaze though. By manual I mean like this:

def requestManual(uri: Uri, body: Json): F[String] = {
  val client: Client[F] = ???

  for {
    hBody    <- http4sBody(body.noSpaces)
    request  =  Request(method = Method.POST, uri = uri, headers = Headers(`Content-Type`(MediaType.application.json)), body = hBody)
    response <- client.expect[String](request)
  } yield response
}

def http4sBody(body: String)(implicit encoder: EntityEncoder[F, String]): F[EntityBody[F]] =
  F.pure(encoder.toEntity(body).body)  // I have tried this as F.delay and it does not affect the result

I know it's weird to send a request in the above method, I've cut down some project code to end up with this. The fact that it works for Jetty and Blaze makes me think while weird it's not necessarily wrong.

The same request using the DSL works under all client implementations (not with client logger enabled using OkHttp though, see README):

def requestDsl(uri: Uri, body: Json): F[String] = {
  val client: Client[F] = ???

  client.expect[String](POST(body, uri))

I think this (and http4s/http4s#2346) is caused because I don't specify the content-length manually and the fact that OkHttpBuilder and AsyncHttpClient blindly trust this (so use an empty body builder). Blaze at least (I haven't chased the Jetty code through) uses the actual request body to build the header (and the body).

OkHttpBuilder:
https://github.com/http4s/http4s/blob/6cdec9d2154bc567270aaab886217e03b9ac591a/okhttp-client/src/main/scala/org/http4s/client/okhttp/OkHttpBuilder.scala#L152

AsyncHttpClient
https://github.com/http4s/http4s/blob/6cdec9d2154bc567270aaab886217e03b9ac591a/async-http-client/src/main/scala/org/http4s/client/asynchttpclient/AsyncHttpClient.scala#L137

Blaze:
https://github.com/http4s/http4s/blob/6cdec9d2154bc567270aaab886217e03b9ac591a/blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala#L122

I'm also having this problem with OkHttp. I worked around it by providing the body in a separate request.withBodyStream(...) call - that also works.

Sorry correction - it appears that it's in fact the addition of content-length header, as you suggested, that fixed this, not withBodyStream as I said previously. :(

This seems like something that would be cleaned up by http4s/http4s#4509.