finagle-http: Fix body and header in 204 and 304

Problem

We currently have two problems. Firstly, as described in RFC2616[1] and
RFC7231[2], 204 and 304 responses must not contain a message-body. This
requirement was not enforced in finagle, i.e., a message-body can be
added. Moreover, a Content-Length header field can be added to these responses,
which is not allowed as mentioned in RFC7230#section-3.3.2[3].

Solution

First of all, we can replace a message-body(which we call content in finagle)
with Buffers.Empty if the response status code is either 204 or 304 to satisfy
RFC7231. As a response status code seems to be finally determined at some point
in HttpServerDispatcher, we can do this in HttpServerDispatcher. Secondly, we
need to make sure not to add a Content-Length header field in a 204 response
to follow RFC7230#section-3.3.2. With regard to a 304 response, a server May
send a Content-Length header as mentioned in the same section. Thus it'd be
better for us to allow users to set the field if they like.

Result

We follow the specification of RFC with regard to a message-header and
message-body except that we have not added some header fields equired in a
response with status code 304 to satisfy RFC7232#section-4.1[4]; Any of the
following header fields must be generated: Cache-Control, Content-Location,
Date, ETag, Expires, and Vary.

[1] https://tools.ietf.org/html/rfc2616
[2] https://tools.ietf.org/html/rfc7231
[3] https://tools.ietf.org/html/rfc7230#section-3.3.2
[4] https://tools.ietf.org/html/rfc7232#section-4.1

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

@@ -8,19 +8,16 @@ import com.twitter.finagle.filter.PayloadSizeFilter
 import com.twitter.finagle.http._
 import com.twitter.finagle.http.codec.{HttpClientDispatcher, HttpServerDispatcher}
 import com.twitter.finagle.http.exp.StreamTransport
-import com.twitter.finagle.http.filter.{ClientContextFilter, HttpNackFilter,
-  ServerContextFilter}
-import com.twitter.finagle.http.netty.{Netty3ClientStreamTransport, Netty3ServerStreamTransport,
-  Netty3HttpTransporter, Netty3HttpListener}
+import com.twitter.finagle.http.filter.{ClientContextFilter, HttpNackFilter, ModifyContentAndHeaderFieldFilter, ServerContextFilter}
+import com.twitter.finagle.http.netty.{Netty3ClientStreamTransport, Netty3HttpListener, Netty3HttpTransporter, Netty3ServerStreamTransport}
 import com.twitter.finagle.netty3._
-import com.twitter.finagle.param.{Monitor => _, ResponseClassifier => _, ExceptionStatsHandler => _,
-  Tracer => _, _}
+import com.twitter.finagle.param.{ExceptionStatsHandler => _, Monitor => _, ResponseClassifier => _, Tracer => _, _}
 import com.twitter.finagle.server._
 import com.twitter.finagle.service.RetryBudget
 import com.twitter.finagle.stats.{ExceptionStatsHandler, StatsReceiver}
 import com.twitter.finagle.tracing._
 import com.twitter.finagle.transport.Transport
-import com.twitter.util.{Duration, Future, StorageUnit, Monitor}
+import com.twitter.util.{Duration, Future, Monitor, StorageUnit}
 import java.net.SocketAddress
 
 /**
@@ -292,6 +289,7 @@ object Http extends Client[Request, Response] with HttpRichClient
       StackServer.newStack
         .replace(TraceInitializerFilter.role, new HttpServerTraceInitializer[Request, Response])
         .replace(StackServer.Role.preparer, HttpNackFilter.module)
+        .prepend(ModifyContentAndHeaderFieldFilter.module)
         .prepend(nonChunkedPayloadSize)
         .prepend(ServerContextFilter.module)
   }

finagle-http/src/main/scala/com/twitter/finagle/http/codec/HttpServerDispatcher.scala

@@ -3,7 +3,7 @@ package com.twitter.finagle.http.codec
 import com.twitter.finagle.Service
 import com.twitter.finagle.http._
 import com.twitter.finagle.http.exp.{GenSerialServerDispatcher, StreamTransport}
-import com.twitter.finagle.stats.{StatsReceiver, RollupStatsReceiver}
+import com.twitter.finagle.stats.{RollupStatsReceiver, StatsReceiver}
 import com.twitter.logging.Logger
 import com.twitter.util.{Future, Promise, Throwables}
 
@@ -31,6 +31,11 @@ private[finagle] class HttpServerDispatcher(
     service.close()
   }
 
+  private def mayHaveContent(status: Status): Boolean = status match {
+    case Status.NoContent | Status.NotModified => false
+    case _ => true
+  }
+
   protected def dispatch(m: Request): Future[Response] = m match {
     case badReq: BadReq =>
       val resp = badReq match {
@@ -66,6 +71,7 @@ private[finagle] class HttpServerDispatcher(
 
   protected def handle(rep: Response): Future[Unit] = {
     setKeepAlive(rep, !isClosing)
+
     if (rep.isChunked) {
       // We remove content length here in case the content is later
       // compressed. This is a pretty bad violation of modularity;
@@ -93,7 +99,7 @@ private[finagle] class HttpServerDispatcher(
       p
     } else {
       // Ensure Content-Length is set if not chunked
-      if (!rep.contentLength.isDefined)
+      if (mayHaveContent(rep.status) && rep.contentLength.isEmpty)
         rep.contentLength = rep.content.length
 
       trans.write(rep)

finagle-http/src/main/scala/com/twitter/finagle/http/filter/ModifyContentAndHeaderFieldFilter.scala

@@ -0,0 +1,54 @@
+package com.twitter.finagle.http.filter
+
+import com.twitter.finagle.http.{Fields, Request, Response, Status}
+import com.twitter.finagle._
+import com.twitter.util.Future
+
+/**
+ * Modify Content and header field filter. Remove message-body and modify a Content-Length header field if necessary.
+ *
+ * @see [[https://tools.ietf.org/html/rfc2616 "Hypertext Transfer Protocol -- HTTP/1.1"]]
+ * @see [[https://tools.ietf.org/html/rfc7230 "Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing"]]
+ * section-3.3.2 on `Content-Length` for more information.
+ */
+private[finagle] class ModifyContentAndHeaderFieldFilter[Req <: Request] extends SimpleFilter[Req, Response] {
+  import ModifyContentAndHeaderFieldFilter._
+
+  def apply(request: Req, service: Service[Req, Response]): Future[Response] = {
+    service(request) map { rep =>
+      if (mustNotIncludeContent(rep.status)) {
+        rep.clearContent()
+
+        if (rep.status == Status.NoContent)
+          rep.headerMap.remove(Fields.ContentLength)
+
+        //Make sure a Content-Length header field is set with a value of 0 if it is explicitly sent for a 304 response
+        if (rep.contentLength.isDefined)
+          rep.headerMap.set(Fields.ContentLength, "0")
+      }
+
+      rep
+    }
+  }
+}
+
+private[finagle] object ModifyContentAndHeaderFieldFilter extends HeadFilter[Request] {
+
+  val Role = Stack.Role("ModifyContentAndHeaderField")
+  val Description = "Remove message-body and modify a Content-Length header field if necessary"
+
+  private def mustNotIncludeContent(status: Status): Boolean = status match {
+    case Status.NoContent | Status.NotModified => true
+    case _ => false
+  }
+
+  def module: Stackable[ServiceFactory[Request, Response]] =
+    new Stack.Module0[ServiceFactory[Request, Response]] {
+      val role = Role
+      val description = Description
+
+      def make(next: ServiceFactory[Request, Response]): ServiceFactory[Request, Response] = {
+        new ModifyContentAndHeaderFieldFilter().andThen(next)
+      }
+    }
+}

finagle-http/src/test/scala/com/twitter/finagle/http/AbstractEndToEndTest.scala

@@ -8,14 +8,17 @@ import com.twitter.finagle.builder.{ClientBuilder, ServerBuilder}
 import com.twitter.finagle.context.Contexts
 import com.twitter.finagle.http.service.HttpResponseClassifier
 import com.twitter.finagle.param.Stats
-import com.twitter.finagle.service.{ResponseClass, FailureAccrualFactory}
-import com.twitter.finagle.stats.{NullStatsReceiver, InMemoryStatsReceiver, StatsReceiver}
+import com.twitter.finagle.service.{FailureAccrualFactory, ResponseClass}
+import com.twitter.finagle.stats.{InMemoryStatsReceiver, NullStatsReceiver, StatsReceiver}
 import com.twitter.finagle.tracing.Trace
 import com.twitter.io.{Buf, Reader, Writer}
 import com.twitter.util.{Await, Closable, Future, JavaTimer, Promise, Return, Throw, Time}
 import java.io.{PrintWriter, StringWriter}
 import java.net.{InetAddress, InetSocketAddress}
+
+import org.jboss.netty.buffer.ChannelBuffers
 import org.scalatest.{BeforeAndAfter, FunSuite}
+
 import scala.language.reflectiveCalls
 
 abstract class AbstractEndToEndTest extends FunSuite with BeforeAndAfter {
@@ -31,6 +34,7 @@ abstract class AbstractEndToEndTest extends FunSuite with BeforeAndAfter {
   object MaxHeaderSize extends Feature
   object SetContentLength extends Feature
   object CompressedContent extends Feature
+  object NoBody extends Feature
 
   var saveBase: Dtab = Dtab.empty
   private val statsRecv: InMemoryStatsReceiver = new InMemoryStatsReceiver()
@@ -372,6 +376,158 @@ abstract class AbstractEndToEndTest extends FunSuite with BeforeAndAfter {
       assert(statsRecv.stat("server", "response_payload_bytes")() == Seq(20.0f))
       client.close()
     }
+
+    testIfImplemented(NoBody)(name + ": must not have a message body nor Content-Length when a 204 response is returned") {
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", statusCodeSvc)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NoContent)), 1.second)
+
+      assert(rep.statusCode == Status.NoContent.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.isEmpty)
+
+      client.close()
+      server.close()
+    }
+
+    testIfImplemented(NoBody)(name +
+      ": must not have a message body nor Content-Length when a 304 response is returned") {
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", statusCodeSvc)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NotModified)), 1.second)
+
+      assert(rep.statusCode == Status.NotModified.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.isEmpty)
+
+      client.close()
+      server.close()
+    }
+
+    testIfImplemented(NoBody)(name +
+      ": must not have a message body nor Content-Length when a 204 response with a non-empty body is returned") {
+      val service = new HttpService {
+        def apply(request: Request) = {
+          val statusCode = request.getIntParam("statusCode", Status.BadRequest.code)
+          val rep = Response(Status.fromCode(statusCode))
+          rep.contentString = "Hello, world!"
+          Future.value(rep)
+        }
+      }
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", service)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NoContent)), 1.second)
+
+      assert(rep.statusCode == Status.NoContent.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.isEmpty)
+
+      client.close()
+      server.close()
+    }
+
+    testIfImplemented(NoBody)(name +
+      ": must not have a message body nor Content-Length when a 304 response with a non-empty body is returned") {
+      val service = new HttpService {
+        def apply(request: Request) = {
+          val statusCode = request.getIntParam("statusCode", Status.BadRequest.code)
+          val rep = Response(Status.fromCode(statusCode))
+          rep.contentString = "Hello, world!"
+          Future.value(rep)
+        }
+      }
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", service)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NotModified)), 1.second)
+
+      assert(rep.statusCode == Status.NotModified.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.isEmpty)
+
+      client.close()
+      server.close()
+    }
+
+    testIfImplemented(NoBody)(name +
+      ": must not have a message body nor Content-Length when a 204 response with an explicit Content-Length header is returned") {
+      val service = new HttpService {
+        def apply(request: Request) = {
+          val statusCode = request.getIntParam("statusCode", Status.BadRequest.code)
+          val rep = Response(Status.fromCode(statusCode))
+          rep.headerMap.set(Fields.ContentLength, "100")
+          Future.value(rep)
+        }
+      }
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", service)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NoContent)), 1.second)
+
+      assert(rep.statusCode == Status.NoContent.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.isEmpty)
+
+      client.close()
+      server.close()
+    }
+
+    testIfImplemented(NoBody)(name +
+      ": must not have a message body but have a Content-Length with a value of 0" +
+      " when a 304 response with an explicit Content-Length header is returned") {
+      val service = new HttpService {
+        def apply(request: Request) = {
+          val statusCode = request.getIntParam("statusCode", Status.BadRequest.code)
+          val rep = Response(Status.fromCode(statusCode))
+          rep.headerMap.set(Fields.ContentLength, "100")
+          Future.value(rep)
+        }
+      }
+      val server = finagle.Http.server
+        .configured(param.Stats(NullStatsReceiver))
+        .serve("localhost:*", service)
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = finagle.Http.client
+        .configured(param.Stats(statsRecv))
+        .newService("%s:%d".format(addr.getHostName, addr.getPort), "client")
+      val rep = Await.result(client(requestWith(Status.NotModified)), 1.second)
+
+      assert(rep.statusCode == Status.NotModified.code)
+      assert(!rep.httpMessage.isChunked)
+      assert(rep.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(rep.contentLength.contains(0))
+
+      client.close()
+      server.close()
+    }
   }
 
   def streaming(name: String)(connect: HttpService => HttpService) {

finagle-http2/src/test/scala/com/twitter/finagle/http2/EndToEndTest.scala

@@ -27,8 +27,9 @@ class EndToEndTest extends AbstractEndToEndTest {
     TooLongStream,
     SetContentLength,
     TooLongFixed,
-    CompressedContent // these tests pass but only because the server ignores
-                      // the compression param and doesn't compress content.
+    CompressedContent, // these tests pass but only because the server ignores
+                       // the compression param and doesn't compress content.
+    NoBody
   )
   def featureImplemented(feature: Feature): Boolean = !featuresToBeImplemented(feature)
 }