Repository: netty/netty
Issue: Add DefaultHttpMessage constructor that takes a HttpHeaders instance
Labels: improvement

Netty version: 4.0.32.Final

Context:
In AHC, we have our own builder for requests, where we internally store headers into a `HttpHeaders` instance.

Sadly, there's no way to directly pass a `HttpHeaders` instance to `DefaultHttpMessage` hierarchy and we have to set our instance into the `DefaultHttpMessage`'s internal one that's been eagerly created.

This results is an excessive allocation that could be avoided.

Please let me know if you're interested in a PR, and which branch to target.

Regards


Issue comments:
If you can make it with breakage use 4.0

> Am 05.10.2015 um 17:36 schrieb Stephane Landelle notifications@github.com:
> 
> Netty version: 4.0.32.Final
> 
> Context:
> In AHC, we have our own builder for requests, where we internally store headers into a HttpHeaders instance.
> 
> Sadly, there's no way to directly pass a HttpHeaders instance to DefaultHttpMessage hierarchy and we have to set our instance into the DefaultHttpMessage's internal one that's been eagerly created.
> 
> This results is an excessive allocation that could be avoided.
> 
> Please let me know if you're interested in a PR, and which branch to target.
> 
> Regards
> 
> —
> Reply to this email directly or view it on GitHub.

Will do :)

> If you can make it with breakage use 4.0

Just to clarify ... If you can make it **without** breaking use 4.0. @normanmaurer ?

lol.. yes. Fat fingers :D

Yeah, that made me smile, but I got it right, no worries ;-)

@slandelle ping :) 

Fixed by https://github.com/netty/netty/pull/4639


Pull request review comments:
ObjectUtils.checkNonNull(...)

Replace with ObjectUtils.checkNonNull(...)

the getStatus -> the status

getMethod -> method


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
index 2ca9329e7c6..285763e5448 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpRequest.java
@@ -18,6 +18,7 @@
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.util.IllegalReferenceCountException;
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
  * Default implementation of {@link FullHttpRequest}.
@@ -25,7 +26,7 @@
 public class DefaultFullHttpRequest extends DefaultHttpRequest implements FullHttpRequest {
     private final ByteBuf content;
     private final HttpHeaders trailingHeader;
-    private final boolean validateHeaders;
+
     /**
      * Used to cache the value of the hash code and avoid {@link IllegalRefCountException}.
      */
@@ -46,12 +47,15 @@ public DefaultFullHttpRequest(HttpVersion httpVersion, HttpMethod method, String
     public DefaultFullHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri,
                                   ByteBuf content, boolean validateHeaders) {
         super(httpVersion, method, uri, validateHeaders);
-        if (content == null) {
-            throw new NullPointerException("content");
-        }
-        this.content = content;
+        this.content = checkNotNull(content, "content");
         trailingHeader = new DefaultHttpHeaders(validateHeaders);
-        this.validateHeaders = validateHeaders;
+    }
+
+    public DefaultFullHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri,
+            ByteBuf content, HttpHeaders headers, HttpHeaders trailingHeader) {
+        super(httpVersion, method, uri, headers);
+        this.content = checkNotNull(content, "content");
+        this.trailingHeader = checkNotNull(trailingHeader, "trailingHeader");
     }
 
     @Override
@@ -137,13 +141,12 @@ public FullHttpRequest setUri(String uri) {
      * @return A copy of this object
      */
     private FullHttpRequest copy(boolean copyContent, ByteBuf newContent) {
-        DefaultFullHttpRequest copy = new DefaultFullHttpRequest(
+        return new DefaultFullHttpRequest(
                 protocolVersion(), method(), uri(),
                 copyContent ? content().copy() :
-                        newContent == null ? Unpooled.buffer(0) : newContent);
-        copy.headers().set(headers());
-        copy.trailingHeaders().set(trailingHeaders());
-        return copy;
+                        newContent == null ? Unpooled.buffer(0) : newContent,
+                headers(),
+                trailingHeaders());
     }
 
     @Override
@@ -158,11 +161,8 @@ public FullHttpRequest copy() {
 
     @Override
     public FullHttpRequest duplicate() {
-        DefaultFullHttpRequest duplicate = new DefaultFullHttpRequest(
-                protocolVersion(), method(), uri(), content().duplicate(), validateHeaders);
-        duplicate.headers().set(headers());
-        duplicate.trailingHeaders().set(trailingHeaders());
-        return duplicate;
+        return new DefaultFullHttpRequest(
+                protocolVersion(), method(), uri(), content().duplicate(), headers(), trailingHeaders());
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
index 5ad6058daf9..a192ba99585 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultFullHttpResponse.java
@@ -28,7 +28,7 @@ public class DefaultFullHttpResponse extends DefaultHttpResponse implements Full
 
     private final ByteBuf content;
     private final HttpHeaders trailingHeaders;
-    private final boolean validateHeaders;
+
     /**
      * Used to cache the value of the hash code and avoid {@link IllegalRefCountException}.
      */
@@ -62,7 +62,13 @@ public DefaultFullHttpResponse(HttpVersion version, HttpResponseStatus status,
         this.content = checkNotNull(content, "content");
         this.trailingHeaders = singleFieldHeaders ? new CombinedHttpHeaders(validateHeaders)
                                                   : new DefaultHttpHeaders(validateHeaders);
-        this.validateHeaders = validateHeaders;
+    }
+
+    public DefaultFullHttpResponse(HttpVersion version, HttpResponseStatus status,
+            ByteBuf content, HttpHeaders headers, HttpHeaders trailingHeaders) {
+        super(version, status, headers);
+        this.content = checkNotNull(content, "content");
+        this.trailingHeaders = checkNotNull(trailingHeaders, "trailingHeaders");
     }
 
     @Override
@@ -142,13 +148,12 @@ public FullHttpResponse setStatus(HttpResponseStatus status) {
      * @return A copy of this object
      */
     private FullHttpResponse copy(boolean copyContent, ByteBuf newContent) {
-        DefaultFullHttpResponse copy = new DefaultFullHttpResponse(
+        return new DefaultFullHttpResponse(
                 protocolVersion(), status(),
                 copyContent ? content().copy() :
-                        newContent == null ? Unpooled.buffer(0) : newContent);
-        copy.headers().set(headers());
-        copy.trailingHeaders().set(trailingHeaders());
-        return copy;
+                        newContent == null ? Unpooled.buffer(0) : newContent,
+                headers(),
+                trailingHeaders());
     }
 
     @Override
@@ -163,11 +168,8 @@ public FullHttpResponse copy() {
 
     @Override
     public FullHttpResponse duplicate() {
-        DefaultFullHttpResponse duplicate = new DefaultFullHttpResponse(protocolVersion(), status(),
-                content().duplicate(), validateHeaders);
-        duplicate.headers().set(headers());
-        duplicate.trailingHeaders().set(trailingHeaders());
-        return duplicate;
+        return new DefaultFullHttpResponse(protocolVersion(), status(),
+                content().duplicate(), headers(), trailingHeaders());
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpMessage.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpMessage.java
index 82fd0cb4fbb..5a6a45a376d 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpMessage.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpMessage.java
@@ -36,9 +36,17 @@ protected DefaultHttpMessage(final HttpVersion version) {
      * Creates a new instance.
      */
     protected DefaultHttpMessage(final HttpVersion version, boolean validateHeaders, boolean singleFieldHeaders) {
+        this(version,
+                singleFieldHeaders ? new CombinedHttpHeaders(validateHeaders)
+                                   : new DefaultHttpHeaders(validateHeaders));
+    }
+
+    /**
+     * Creates a new instance.
+     */
+    protected DefaultHttpMessage(final HttpVersion version, HttpHeaders headers) {
         this.version = checkNotNull(version, "version");
-        headers = singleFieldHeaders ? new CombinedHttpHeaders(validateHeaders)
-                                     : new DefaultHttpHeaders(validateHeaders);
+        this.headers = checkNotNull(headers, "headers");
     }
 
     @Override
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
index 7844688d35f..84be3bb72c9 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
@@ -29,7 +29,7 @@ public class DefaultHttpRequest extends DefaultHttpMessage implements HttpReques
      * Creates a new instance.
      *
      * @param httpVersion the HTTP version of the request
-     * @param method      the HTTP getMethod of the request
+     * @param method      the HTTP method of the request
      * @param uri         the URI or path of the request
      */
     public DefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri) {
@@ -40,7 +40,7 @@ public DefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri
      * Creates a new instance.
      *
      * @param httpVersion       the HTTP version of the request
-     * @param method            the HTTP getMethod of the request
+     * @param method            the HTTP method of the request
      * @param uri               the URI or path of the request
      * @param validateHeaders   validate the header names and values when adding them to the {@link HttpHeaders}
      */
@@ -50,6 +50,20 @@ public DefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri
         this.uri = checkNotNull(uri, "uri");
     }
 
+    /**
+     * Creates a new instance.
+     *
+     * @param httpVersion       the HTTP version of the request
+     * @param method            the HTTP method of the request
+     * @param uri               the URI or path of the request
+     * @param headers           the Headers for this Request
+     */
+    public DefaultHttpRequest(HttpVersion httpVersion, HttpMethod method, String uri, HttpHeaders headers) {
+        super(httpVersion, headers);
+        this.method = checkNotNull(method, "method");
+        this.uri = checkNotNull(uri, "uri");
+    }
+
     @Override
     @Deprecated
     public HttpMethod getMethod() {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
index 4ee54a3f3f1..86858108a27 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
@@ -15,6 +15,7 @@
  */
 package io.netty.handler.codec.http;
 
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
  * The default {@link HttpResponse} implementation.
@@ -27,7 +28,7 @@ public class DefaultHttpResponse extends DefaultHttpMessage implements HttpRespo
      * Creates a new instance.
      *
      * @param version the HTTP version of this response
-     * @param status  the getStatus of this response
+     * @param status  the status of this response
      */
     public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status) {
         this(version, status, true, false);
@@ -37,7 +38,7 @@ public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status) {
      * Creates a new instance.
      *
      * @param version           the HTTP version of this response
-     * @param status            the getStatus of this response
+     * @param status            the status of this response
      * @param validateHeaders   validate the header names and values when adding them to the {@link HttpHeaders}
      */
     public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status, boolean validateHeaders) {
@@ -48,7 +49,7 @@ public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status, boole
      * Creates a new instance.
      *
      * @param version           the HTTP version of this response
-     * @param status            the getStatus of this response
+     * @param status            the status of this response
      * @param validateHeaders   validate the header names and values when adding them to the {@link HttpHeaders}
      * @param singleFieldHeaders {@code true} to check and enforce that headers with the same name are appended
      * to the same entry and comma separated.
@@ -59,10 +60,19 @@ public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status, boole
     public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status, boolean validateHeaders,
                                boolean singleFieldHeaders) {
         super(version, validateHeaders, singleFieldHeaders);
-        if (status == null) {
-            throw new NullPointerException("status");
-        }
-        this.status = status;
+        this.status = checkNotNull(status, "status");
+    }
+
+    /**
+     * Creates a new instance.
+     *
+     * @param version           the HTTP version of this response
+     * @param status            the status of this response
+     * @param headers           the headers for this HTTP Response
+     */
+    public DefaultHttpResponse(HttpVersion version, HttpResponseStatus status, HttpHeaders headers) {
+        super(version, headers);
+        this.status = checkNotNull(status, "status");
     }
 
     @Override


Test patch: