Http4s: Client resource leak if response mapping throws an exception

[jfwilson]

If the response mapping function f: Response => Task[A] passed into Client.fetch[A] or Client.get[A] throws an exception, the DisposableResponse is not disposed of.

Symptom

If using SimpleHttp1Client, the connection is not shutdown and the warning message Client was not shut down and could result in a resource leak appears in the logs.

How to reproduce

import org.http4s.Status.ResponseClass.Successful
import scalaz.concurrent.Task

val client = org.http4s.client.blaze.defaultClient

val doRequest = client.get("http://example.com/not-found") {
  case Successful(_) => Task.now("Success!")
}
println(doRequest.unsafePerformSyncAttempt) // -\/(MatchError) due to 404 response

// Now trigger/wait for GC...

client.shutdownNow()

Cause

The client passes the function directly to DisposableResponse.apply method, which is supposed to dispose of the response after the function completes. However if the function throws an exception, the response is never disposed: https://github.com/http4s/http4s/blob/ffe9a04fbb878693e8e17c113e04658391135f35/client/src/main/scala/org/http4s/client/Client.scala#L24

[rossabaker]

Thanks for an excellent write-up.

Two things come to mind here:

1. An unmatched response should probably go into a failed task instead of throwing.
2. client.get(req) { case _ => sys.error("Don't do this! But people do anyway!") } should be handled.

[agustafson]

Thanks for fixing that so quickly @rossabaker! Any chance you can put that in a 0.14.x release too? :)

[rossabaker]

Yes, this and #663 will be released as 0.14.3 as soon as they get a review. :)

[agustafson]

Perfect, thanks @rossabaker 👍