Merge pull request #2693 from rossabaker/issue-2691

Release connections before following redirects

client/src/main/scala/org/http4s/client/middleware/FollowRedirect.scala

@@ -40,7 +40,7 @@ object FollowRedirect {
   def apply[F[_]](
       maxRedirects: Int,
       sensitiveHeaderFilter: CaseInsensitiveString => Boolean = Headers.SensitiveHeaders)(
-      client: Client[F])(implicit F: Bracket[F, Throwable]): Client[F] = {
+      client: Client[F])(implicit F: Concurrent[F]): Client[F] = {
 
     def nextRequest(req: Request[F], uri: Uri, method: Method): Request[F] = {
       // https://tools.ietf.org/html/rfc7231#section-7.1.
@@ -65,22 +65,27 @@ object FollowRedirect {
       clearBodyFromGetHead(stripSensitiveHeaders(req).withMethod(method).withUri(nextUri))
     }
 
-    def prepareLoop(req: Request[F], redirects: Int): Resource[F, Response[F]] =
-      client.run(req).flatMap { resp =>
-        (methodForRedirect(req, resp), resp.headers.get(Location)) match {
-          case (Some(method), Some(loc)) if redirects < maxRedirects =>
-            val nextReq = nextRequest(req, loc.uri, method)
-            prepareLoop(nextReq, redirects + 1).map { response =>
-              // prepend because `prepareLoop` is recursive
-              response.withAttribute(redirectUrisKey, nextReq.uri +: getRedirectUris(response))
-            }
-          case _ => resp.pure[Resource[F, ?]]
-          // IF the response is missing the Location header, OR there is no method to redirect,
-          // OR we have exceeded max number of redirections, THEN we redirect no more
-        }
+    def prepareLoop(req: Request[F], redirects: Int): F[Resource[F, Response[F]]] =
+      F.continual(client.run(req).allocated) {
+        case Right((resp, dispose)) =>
+          (methodForRedirect(req, resp), resp.headers.get(Location)) match {
+            case (Some(method), Some(loc)) if redirects < maxRedirects =>
+              val nextReq = nextRequest(req, loc.uri, method)
+              dispose >> prepareLoop(nextReq, redirects + 1).map(_.map { response =>
+                // prepend because `prepareLoop` is recursive
+                response.withAttribute(redirectUrisKey, nextReq.uri +: getRedirectUris(response))
+              })
+            case _ =>
+              // IF the response is missing the Location header, OR there is no method to redirect,
+              // OR we have exceeded max number of redirections, THEN we redirect no more
+              Resource.make(resp.pure[F])(_ => dispose).pure[F]
+          }
+
+        case Left(e) =>
+          F.raiseError(e)
       }
 
-    Client(prepareLoop(_, 0))
+    Client(req => Resource.suspend(prepareLoop(req, 0)))
   }
 
   private def methodForRedirect[F[_]](req: Request[F], resp: Response[F]): Option[Method] =

client/src/test/scala/org/http4s/client/middleware/FollowRedirectSpec.scala

@@ -3,6 +3,7 @@ package client
 package middleware
 
 import cats.effect._
+import cats.effect.concurrent.Semaphore
 import cats.implicits._
 import fs2._
 import java.util.concurrent.atomic._
@@ -167,6 +168,18 @@ class FollowRedirectSpec extends Http4sSpec with Http4sClientDsl[IO] with Tables
       disposed must_== 2 // one for the original, one for the redirect
     }
 
+    "Not hang when redirecting" in {
+      Semaphore[IO](2).flatMap { semaphore =>
+        def f(req: Request[IO]) =
+          Resource.make(semaphore.tryAcquire.flatMap {
+            case true => app.run(req)
+            case false => IO.raiseError(new IllegalStateException("Exhausted all connections"))
+          })(_ => semaphore.release)
+        val client = FollowRedirect(3)(Client(f))
+        client.status(Request[IO](uri = uri("http://localhost/loop/3")))
+      } must returnValue(Status.Ok)
+    }
+
     "Not send sensitive headers when redirecting to a different authority" in {
       val req = PUT(
         "Don't expose mah secrets!",