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 have to clear a message-body, which we call content in finagle,
if the response status code is either 204 or 304 to satisfy RFC7231. 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 with a value of 0 if they like.
Most importantly, we make sure that these things are achieved and the new system
is bugwards-compatible by provding a new filter.[4]

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 required in a
response with status code 304 to satisfy RFC7232#section-4.1[5]; Any of the
following header fields must be generated: Cache-Control, Content-Location,
Date, ETag, Expires, and Vary.
As we achieve this by creating a new filter and set it to Http.scala[6], users
who are not willing to make use of the functionality can easily disable it
by removing the filter.

[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] twitter#521 (comment)
[5] https://tools.ietf.org/html/rfc7232#section-4.1
[6] https://github.com/monkey-mas/finagle/blob/finagle-6.35.0/finagle-http/src/main/scala/com/twitter/finagle/Http.scala#L298-L304

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

@@ -8,13 +8,10 @@ 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}
@@ -306,6 +303,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 {
@@ -93,7 +98,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,55 @@
+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 a content and header field filter in a 204 and 304 response.
+ * Remove a 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 a 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()
@@ -373,6 +377,158 @@ abstract class AbstractEndToEndTest extends FunSuite with BeforeAndAfter {
       assert(statsRecv.stat("server", "response_payload_bytes")() == Seq(20.0f))
       Await.ready(client.close(), 5.seconds)
     }
+
+    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)
 }