HttpServerResponse content headers improvements - fixes #2630 - fixes #…

…2607
eclipse-vertx · Sep 19, 2018 · 20c2669 · 20c2669
1 parent e1f6fed
commit 20c2669
Show file tree

Hide file tree

Showing 9 changed files with 258 additions and 109 deletions.
diff --git a/src/main/asciidoc/java/override/dependencies.adoc b/src/main/asciidoc/java/override/dependencies.adoc
@@ -8,7 +8,7 @@ project descriptor to access the Vert.x Core API:
 <dependency>
   <groupId>io.vertx</groupId>
   <artifactId>vertx-core</artifactId>
-  <version>3.5.3</version>
+  <version>3.5.4-SNAPSHOT</version>
 </dependency>
 ----
 
@@ -17,6 +17,6 @@ project descriptor to access the Vert.x Core API:
 [source,groovy,subs="+attributes"]
 ----
 dependencies {
-  compile 'io.vertx:vertx-core:3.5.3'
+  compile 'io.vertx:vertx-core:3.5.4-SNAPSHOT'
 }
 ----
diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerConnection.java b/src/main/java/io/vertx/core/http/impl/Http2ServerConnection.java
@@ -34,6 +34,8 @@
 import java.net.URISyntaxException;
 import java.util.ArrayDeque;
 
+import static io.vertx.core.spi.metrics.Metrics.METRICS_ENABLED;
+
 /**
  * @author <a href="mailto:[email protected]">Julien Viet</a>
  */
@@ -277,7 +279,10 @@ void handleClose() {
 
     void complete() {
       synchronized (Http2ServerConnection.this) {
-        response = new Http2ServerResponseImpl(Http2ServerConnection.this, this, method, uri, true, contentEncoding);
+        response = new Http2ServerResponseImpl(Http2ServerConnection.this, this, method, true, contentEncoding, null);
+        if (METRICS_ENABLED && metrics != null) {
+          response.metric(metrics.responsePushed(conn.metric(), method, uri, response));
+        }
         completionHandler.complete(response);
       }
     }

diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java b/src/main/java/io/vertx/core/http/impl/Http2ServerRequestImpl.java
@@ -94,8 +94,10 @@ public Http2ServerRequestImpl(Http2ServerConnection conn, Http2Stream stream, Ht
       int idx = serverOrigin.indexOf("://");
       host = serverOrigin.substring(idx + 3);
     }
-    Object metric = (METRICS_ENABLED && metrics != null) ? metrics.requestBegin(conn.metric(), this) : null;
-    this.response = new Http2ServerResponseImpl(conn, this, metric, false, contentEncoding, host);
+    this.response = new Http2ServerResponseImpl(conn, this, method(), false, contentEncoding, host);
+    if (METRICS_ENABLED && metrics != null) {
+      response.metric(metrics.requestBegin(conn.metric(), this));
+    }
   }
 
   @Override

diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponseImpl.java
@@ -17,6 +17,7 @@
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpResponseStatus;
+import io.netty.handler.codec.http.HttpStatusClass;
 import io.netty.handler.codec.http2.DefaultHttp2Headers;
 import io.netty.handler.codec.http2.Http2Headers;
 import io.vertx.codegen.annotations.Nullable;
@@ -26,6 +27,7 @@
 import io.vertx.core.Handler;
 import io.vertx.core.MultiMap;
 import io.vertx.core.buffer.Buffer;
+import io.vertx.core.http.HttpHeaders;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.http.HttpServerResponse;
 import io.vertx.core.http.StreamResetException;
@@ -51,17 +53,18 @@ public class Http2ServerResponseImpl implements HttpServerResponse {
   private final VertxHttp2Stream stream;
   private final ChannelHandlerContext ctx;
   private final Http2ServerConnection conn;
+  private final boolean head;
   private final boolean push;
-  private final Object metric;
   private final String host;
   private Http2Headers headers = new DefaultHttp2Headers();
+  private Object metric;
   private Http2HeadersAdaptor headersMap;
   private Http2Headers trailers;
   private Http2HeadersAdaptor trailedMap;
   private boolean chunked;
   private boolean headWritten;
   private boolean ended;
-  private int statusCode = 200;
+  private HttpResponseStatus status = HttpResponseStatus.OK;
   private String statusMessage; // Not really used but we keep the message for the getStatusMessage()
   private Handler<Void> drainHandler;
   private Handler<Throwable> exceptionHandler;
@@ -73,12 +76,16 @@ public class Http2ServerResponseImpl implements HttpServerResponse {
   private int numPush;
   private boolean inHandler;
 
-  public Http2ServerResponseImpl(Http2ServerConnection conn, VertxHttp2Stream stream, Object metric, boolean push, String contentEncoding, String host) {
-
-    this.metric = metric;
+  public Http2ServerResponseImpl(Http2ServerConnection conn,
+                                 VertxHttp2Stream stream,
+                                 HttpMethod method,
+                                 boolean push,
+                                 String contentEncoding,
+                                 String host) {
     this.stream = stream;
     this.ctx = conn.handlerContext;
     this.conn = conn;
+    this.head = method == HttpMethod.HEAD;
     this.push = push;
     this.host = host;
 
@@ -87,25 +94,8 @@ public Http2ServerResponseImpl(Http2ServerConnection conn, VertxHttp2Stream stre
     }
   }
 
-  public Http2ServerResponseImpl(
-      Http2ServerConnection conn,
-      VertxHttp2Stream stream,
-      HttpMethod method,
-      String path,
-      boolean push,
-      String contentEncoding) {
-    this.stream = stream;
-    this.ctx = conn.handlerContext;
-    this.conn = conn;
-    this.push = push;
-    this.host = null;
-
-    if (contentEncoding != null) {
-      putHeader(HttpHeaderNames.CONTENT_ENCODING, contentEncoding);
-    }
-
-    HttpServerMetrics metrics = conn.metrics();
-    this.metric = (METRICS_ENABLED && metrics != null) ? metrics.responsePushed(conn.metric(), method, path, this) : null;
+  void metric(Object metric) {
+    this.metric = metric;
   }
 
   synchronized void beginRequest() {
@@ -152,7 +142,7 @@ public HttpServerResponse exceptionHandler(Handler<Throwable> handler) {
   @Override
   public int getStatusCode() {
     synchronized (conn) {
-      return statusCode;
+      return status.code();
     }
   }
 
@@ -163,7 +153,7 @@ public HttpServerResponse setStatusCode(int statusCode) {
     }
     synchronized (conn) {
       checkHeadWritten();
-      this.statusCode = statusCode;
+      this.status = HttpResponseStatus.valueOf(statusCode);
       return this;
     }
   }
@@ -172,7 +162,7 @@ public HttpServerResponse setStatusCode(int statusCode) {
   public String getStatusMessage() {
     synchronized (conn) {
       if (statusMessage == null) {
-        return HttpResponseStatus.valueOf(statusCode).reasonPhrase();
+        return status.reasonPhrase();
       }
       return statusMessage;
     }
@@ -380,13 +370,26 @@ private void end(ByteBuf chunk) {
     }
   }
 
+  private void sanitizeHeaders() {
+    if (head || status == HttpResponseStatus.NOT_MODIFIED) {
+      headers.remove("transfer-encoding");
+    } else if (status == HttpResponseStatus.RESET_CONTENT) {
+      headers.remove("transfer-encoding");
+      headers.set("content-length", "0");
+    } else if (status.codeClass() == HttpStatusClass.INFORMATIONAL || status == HttpResponseStatus.NO_CONTENT) {
+      headers.remove("transfer-encoding");
+      headers.remove("content-length");
+    }
+  }
+
   private boolean checkSendHeaders(boolean end) {
     if (!headWritten) {
       if (headersEndHandler != null) {
         headersEndHandler.handle(null);
       }
+      sanitizeHeaders();
       headWritten = true;
-      headers.status(Integer.toString(statusCode));
+      headers.status(Integer.toString(status.code())); // Could be optimized for usual case ?
       stream.writeHeaders(headers, end);
       if (end) {
         ctx.flush();
@@ -408,7 +411,7 @@ void write(ByteBuf chunk, boolean end) {
         chunk = Unpooled.EMPTY_BUFFER;
       }
       if (end) {
-        if (!headWritten && !headers.contains(HttpHeaderNames.CONTENT_LENGTH)) {
+        if (!headWritten && !head && status != HttpResponseStatus.NOT_MODIFIED && !headers.contains(HttpHeaderNames.CONTENT_LENGTH)) {
           headers().set(HttpHeaderNames.CONTENT_LENGTH, String.valueOf(chunk.readableBytes()));
         }
         handleEnded(false);

diff --git a/src/main/java/io/vertx/core/http/impl/HttpServerResponseImpl.java b/src/main/java/io/vertx/core/http/impl/HttpServerResponseImpl.java
@@ -69,7 +69,6 @@ public class HttpServerResponseImpl implements HttpServerResponse {
   private Handler<Void> endHandler;
   private Handler<Void> headersEndHandler;
   private Handler<Void> bodyEndHandler;
-  private boolean chunked;
   private boolean closed;
   private final VertxHttpHeaders headers;
   private MultiMap trailers;
@@ -134,7 +133,7 @@ public HttpServerResponseImpl setChunked(boolean chunked) {
       checkValid();
       // HTTP 1.0 does not support chunking so we ignore this if HTTP 1.0
       if (version != HttpVersion.HTTP_1_0) {
-        this.chunked = chunked;
+        headers.set(HttpHeaders.TRANSFER_ENCODING, chunked ? "chunked" : null);
       }
       return this;
     }
@@ -143,7 +142,7 @@ public HttpServerResponseImpl setChunked(boolean chunked) {
   @Override
   public boolean isChunked() {
     synchronized (conn) {
-      return chunked;
+      return HttpHeaders.CHUNKED.equals(headers.get(HttpHeaders.TRANSFER_ENCODING));
     }
   }
 
@@ -443,10 +442,10 @@ private void doSendFile(String filename, long offset, long length, Handler<Async
 
       long contentLength = Math.min(length, file.length() - offset);
       bytesWritten = contentLength;
-      if (!headers.contentTypeSet()) {
+      if (!headers.contains(HttpHeaders.CONTENT_TYPE)) {
         String contentType = MimeMapping.getMimeTypeForFilename(filename);
         if (contentType != null) {
-          putHeader(HttpHeaders.CONTENT_TYPE, contentType);
+          headers.set(HttpHeaders.CONTENT_TYPE, contentType);
         }
       }
       prepareHeaders(bytesWritten);
@@ -555,10 +554,18 @@ private void prepareHeaders(long contentLength) {
     } else if (version == HttpVersion.HTTP_1_1 && !keepAlive) {
       headers.set(HttpHeaders.CONNECTION, HttpHeaders.CLOSE);
     }
-    if (!head) {
-      if (chunked) {
-        headers.set(HttpHeaders.TRANSFER_ENCODING, HttpHeaders.CHUNKED);
-      } else if (!headers.contentLengthSet() && contentLength >= 0) {
+    if (head || status == HttpResponseStatus.NOT_MODIFIED) {
+      // For HEAD request or NOT_MODIFIED response
+      // don't set automatically the content-length
+      // and remove the transfer-encoding
+      headers.remove(HttpHeaders.TRANSFER_ENCODING);
+    } else if (status == HttpResponseStatus.RESET_CONTENT) {
+      // https://github.com/netty/netty/pull/7891
+      headers.remove(HttpHeaders.TRANSFER_ENCODING);
+      headers.set(HttpHeaders.CONTENT_LENGTH, "0");
+    } else {
+      // Set content-length header automatically
+      if (!headers.contains(HttpHeaders.TRANSFER_ENCODING) && !headers.contains(HttpHeaders.CONTENT_LENGTH) && contentLength >= 0) {
         String value = contentLength == 0 ? "0" : String.valueOf(contentLength);
         headers.set(HttpHeaders.CONTENT_LENGTH, value);
       }
@@ -572,7 +579,7 @@ private void prepareHeaders(long contentLength) {
   private HttpServerResponseImpl write(ByteBuf chunk) {
     synchronized (conn) {
       checkValid();
-      if (!headWritten && !chunked && !headers.contentLengthSet()) {
+      if (!headWritten && !headers.contains(HttpHeaders.TRANSFER_ENCODING) && !headers.contains(HttpHeaders.CONTENT_LENGTH)) {
         if (version != HttpVersion.HTTP_1_0) {
           throw new IllegalStateException("You must set the Content-Length header to be the total size of the message "
             + "body BEFORE sending any data if you are not using HTTP chunked encoding.");

diff --git a/src/main/java/io/vertx/core/http/impl/VertxHttpResponseEncoder.java b/src/main/java/io/vertx/core/http/impl/VertxHttpResponseEncoder.java
@@ -42,6 +42,6 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
   protected boolean isContentAlwaysEmpty(HttpResponse msg) {
     // In HttpServerCodec this is tracked via a FIFO queue of HttpMethod
     // here we track it in the assembled response as we don't use HttpServerCodec
-    return msg instanceof AssembledHttpResponse && ((AssembledHttpResponse) msg).head();
+    return (msg instanceof AssembledHttpResponse && ((AssembledHttpResponse) msg).head()) || super.isContentAlwaysEmpty(msg);
   }
 }
diff --git a/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java b/src/main/java/io/vertx/core/http/impl/headers/VertxHttpHeaders.java
@@ -69,14 +69,6 @@ public VertxHttpHeaders() {
     head.before = head.after = head;
   }
 
-  public boolean contentLengthSet() {
-    return contains(io.vertx.core.http.HttpHeaders.CONTENT_LENGTH);
-  }
-
-  public boolean contentTypeSet() {
-    return contains(io.vertx.core.http.HttpHeaders.CONTENT_TYPE);
-  }
-
   @Override
   public VertxHttpHeaders add(CharSequence name, CharSequence value) {
     int h = AsciiString.hashCode(name);
@@ -388,7 +380,7 @@ public VertxHttpHeaders add(CharSequence name, Iterable values) {
   }
 
   @Override
-  public MultiMap set(CharSequence name, CharSequence value) {
+  public VertxHttpHeaders set(CharSequence name, CharSequence value) {
     return set(name.toString(), value.toString());
   }
 
@@ -484,7 +476,7 @@ public HttpHeaders set(String name, Object value) {
 
   @Override
   public HttpHeaders setInt(CharSequence name, int value) {
-    throw new UnsupportedOperationException();
+    return set(name, Integer.toString(value));
   }
 
   @Override