Merge pull request #917 from http4s/connection-closing-0.23

Fix Connection persistence according to RFC 9112

Author: rossabaker

blaze-client/src/main/scala/org/http4s/blaze/client/Http1Connection.scala

@@ -32,6 +32,7 @@ import org.http4s.blaze.pipeline.Command.EOF
 import org.http4s.blazecore.Http1Stage
 import org.http4s.blazecore.IdleTimeoutStage
 import org.http4s.blazecore.util.Http1Writer
+import org.http4s.blazecore.util.NullWriter
 import org.http4s.client.RequestKey
 import org.http4s.headers.Host
 import org.http4s.headers.`Content-Length`
@@ -200,10 +201,8 @@ private final class Http1Connection[F[_]](
           if (userAgent.nonEmpty && !req.headers.contains[`User-Agent`])
             rr << userAgent.get << "\r\n"
 
-          val mustClose: Boolean = req.headers.get[HConnection] match {
-            case Some(conn) => checkCloseConnection(conn, rr)
-            case None => getHttpMinor(req) == 0
-          }
+          val mustClose: Boolean =
+            checkRequestCloseConnection(req.headers.get[HConnection], getHttpMinor(req), NullWriter)
 
           val writeRequest: F[Boolean] = getChunkEncoder(req, mustClose, rr)
             .write(rr, req.body)

blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala

@@ -60,6 +60,7 @@ private[http4s] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] =>
   protected def contentComplete(): Boolean
 
   /** Check Connection header and add applicable headers to response */
+  @deprecated("Use checkConnectionPersistence(Option[Connection], Int, Writer) instead", "0.23.17")
   protected final def checkCloseConnection(conn: Connection, rr: StringWriter): Boolean =
     if (conn.hasKeepAlive) { // connection, look to the request
       logger.trace("Found Keep-Alive header")
@@ -76,6 +77,42 @@ private[http4s] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] =>
       true
     }
 
+  /** Checks whether the connection should be closed per the request's Connection header
+    * and the HTTP version.
+    *
+    * As a side effect, writes a "Connection: close" header to the StringWriter if
+    * the request explicitly requests the connection is closed.
+    *
+    * @see [[https://datatracker.ietf.org/doc/html/rfc9112#name-persistence RFC 9112, Section 9.3, Persistence]]
+    */
+  private[http4s] final def checkRequestCloseConnection(
+      conn: Option[Connection],
+      minorVersion: Int,
+      rr: Writer,
+  ): Boolean =
+    if (conn.fold(false)(_.hasClose)) {
+      logger.trace(s"Closing ${conn} due to explicit close option in request's Connection header")
+      // This side effect doesn't really belong here, but is relied
+      // upon in multiple places as we look for the encoder.  The
+      // related problem of writing the keep-alive header in HTTP/1.0
+      // is handled elsewhere.
+      if (minorVersion >= 1) {
+        rr << "Connection: close\r\n"
+      }
+      true
+    } else if (minorVersion >= 1) {
+      logger.trace(s"Keeping ${conn} alive per default behavior of HTTP >= 1.1")
+      false
+    } else if (conn.fold(false)(_.hasKeepAlive)) {
+      logger.trace(
+        s"Keeping ${conn} alive due to explicit keep-alive option in request's Connection header"
+      )
+      false
+    } else {
+      logger.trace(s"Closing ${conn} per default behavior of HTTP/1.0")
+      true
+    }
+
   /** Get the proper body encoder based on the request */
   protected final def getEncoder(
       req: Request[F],

blaze-core/src/main/scala/org/http4s/blazecore/util/NullWriter.scala

@@ -0,0 +1,27 @@
+/*
+ * Copyright 2014 http4s.org
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.http4s
+package blazecore.util
+
+import org.http4s.util.Writer
+
+/** A writer that does not write.  Not to be confused with an
+  * [[EntityBodyWriter]].
+  */
+private[http4s] object NullWriter extends Writer {
+  def append(s: String): NullWriter.type = NullWriter
+}

blaze-server/src/main/scala/org/http4s/blaze/server/Http1ServerStage.scala

@@ -245,12 +245,9 @@ private[blaze] class Http1ServerStage[F[_]](
     // Need to decide which encoder and if to close on finish
     val closeOnFinish = respConn
       .map(_.hasClose)
-      .orElse {
-        req.headers.get[Connection].map(checkCloseConnection(_, rr))
-      }
       .getOrElse(
-        parser.minorVersion() == 0
-      ) // Finally, if nobody specifies, http 1.0 defaults to close
+        checkRequestCloseConnection(req.headers.get[Connection], parser.minorVersion(), rr)
+      )
 
     // choose a body encoder. Will add a Transfer-Encoding header if necessary
     val bodyEncoder: Http1Writer[F] =

blaze-server/src/test/scala/org/http4s/blaze/server/ServerTestRoutes.scala

@@ -57,13 +57,19 @@ object ServerTestRoutes extends Http4sDsl[IO] {
       // ///////////////////////////////
       (
         "GET /get HTTP/1.0\r\nConnection:close\r\n\r\n",
-        (Status.Ok, Set(length(3), textPlain, connClose), "get"),
+        (Status.Ok, Set(length(3), textPlain), "get"),
       ),
       // ///////////////////////////////
       (
         "GET /get HTTP/1.1\r\nConnection:close\r\n\r\n",
         (Status.Ok, Set(length(3), textPlain, connClose), "get"),
       ),
+      // ///////////////////////////////
+      // Don't close connection on an unrecognized Connection header
+      (
+        "GET /get HTTP/1.1\r\nConnection: fiddle-faddle\r\n\r\n",
+        (Status.Ok, Set(length(3), textPlain), "get"),
+      ),
       ("GET /chunked HTTP/1.1\r\n\r\n", (Status.Ok, Set(textPlain, chunked), "chunk")),
       // ///////////////////////////////
       (
@@ -75,7 +81,7 @@ object ServerTestRoutes extends Http4sDsl[IO] {
       // ///////////////////////////////
       (
         "GET /chunked HTTP/1.0\r\nConnection:Close\r\n\r\n",
-        (Status.Ok, Set(textPlain, connClose), "chunk"),
+        (Status.Ok, Set(textPlain), "chunk"),
       ),
       // ////////////////////////////// Requests with a body //////////////////////////////////////
       (
@@ -90,7 +96,7 @@ object ServerTestRoutes extends Http4sDsl[IO] {
       // ///////////////////////////////
       (
         "POST /post HTTP/1.0\r\nConnection:close\r\nContent-Length:3\r\n\r\nfoo",
-        (Status.Ok, Set(textPlain, length(4), connClose), "post"),
+        (Status.Ok, Set(textPlain, length(4)), "post"),
       ),
       // ///////////////////////////////
       (
@@ -119,7 +125,7 @@ object ServerTestRoutes extends Http4sDsl[IO] {
       // /////////////////////////////// Check corner cases //////////////////
       (
         "GET /twocodings HTTP/1.0\r\nConnection:Close\r\n\r\n",
-        (Status.Ok, Set(textPlain, length(3), connClose), "Foo"),
+        (Status.Ok, Set(textPlain, length(3)), "Foo"),
       ),
       // /////////////// Work with examples that don't have a body //////////////////////
       ("GET /notmodified HTTP/1.1\r\n\r\n", (Status.NotModified, Set(), "")),