Responses to HEAD requests not handled properly by client

[jfwilson]

Responses to HEAD requests typically include Content-Length header, but the body is not sent.

The Blaze client seems to be expecting server to send the body, and holds the connection waiting for data while no data is sent.

Looks like this could be a known issue as there is a TODO comment in the code: https://github.com/http4s/blaze/blob/81d5d0d8f825cf588ff1ef433bf418a386506669/http/src/main/java/org/http4s/blaze/http/http_parser/Http1ClientParser.java#L81

The following code demonstrates the problem:

import org.http4s._
import org.http4s.client.blaze.PooledHttp1Client
import org.http4s.server.blaze.BlazeBuilder
import org.http4s.dsl._

import scalaz.concurrent.Task

object HeadRequestsApp extends App {

  val demoTheProblem = for {
    server <- BlazeBuilder.
      bindLocal(0).
      mountService(HttpService.lift(_ => Ok("Example Body"))).
      start
    uri <- Task.fromDisjunction(Uri.fromString(s"http://${server.address.getHostName}:${server.address.getPort}/"))

    client = PooledHttp1Client(maxTotalConnections = 1) // All requests are to the same host, connection should be reused

    _ <- client.fetch(Request(GET, uri))(printOutResponse).attempt  // Works fine
    _ <- client.fetch(Request(HEAD, uri))(printOutResponse).attempt // Hangs reading EntityBody
    _ <- client.fetch(Request(GET, uri))(printOutResponse).attempt  // Even if EntityBody is not read, connection cannot be reused

    _ <- client.shutdown
    _ <- server.shutdown
  } yield ()

  demoTheProblem.unsafePerformSync

  private def printOutResponse(response: Response): Task[Unit] = Task.suspend {
    println(s"Got ${response.status.renderString} with Content-Length: ${response.contentLength}")
    response.body.runLog.map { chunks =>
      println(s"Actual body length was ${chunks.map(_.length).sum}")
    }
  }
}

[bryce-anderson]

Indeed, this is a problem. I don't think the line noted above my @jfwilson is actually a problem: a ClientParser alone cannot know the nature of the request for which it is trying to parse a response (and I think the TODO needs to be changed to a documentation warning noting that this is a case beyond the reach of the parser).

I suspect where we want to be looking is about here where we need to signal that this was a HEAD request and skip reading the body and close the parser manually if that is the case.

I can look at this more this weekend (maybe sooner, who knows) but I'm happy to offer advice if someone wants to take a swing at contributing a patch!

[jfwilson]

I'm happy to have a go at fixing it, still finding my way around the codebase though so could take me a while!

[rossabaker]

I don't know exactly where to look, but there's usually someone around on Gitter to help with any questions.

[bryce-anderson]

I'm excited to get someone familiar with this code, so I'm happy to help as I can. My first inclination would be to thread a flag (something to the spirit of doesntHaveBody) to this position and then shut down the parser and return an empty body, if appropriate. Care should be taken to honor connection: close headers etc.

It feels like a quick change, the most difficult part being keeping that section readable (maybe its already not?) but there could be surprises, who knows.

[jfwilson]

Sorry have had some other things on recently but taken a look at this today, see this pull request. Happy to make adjustments to the pull request.

[bryce-anderson]

I'm happy that the original issue is solved in http4s propper, but this is still technically a bug for the blaze library. Specifically, the same exact changes need to be taken in here.

@jfwilson, I'm going to leave this open for now and hope to get to it sometime in the future. I don't think that anybody is using the raw blaze http server or client so its not a pressing issue. Thanks again @jfwilson for taking care of it in http4s!