Repository: netty/netty
Issue: SPDY codec broken in 4.0.0.Beta1
Labels: defect

Commit 238e03f75bf7d73b22b475f6f070223857613cd2 broke SPDY; in that commit method() was renamed getMethod().  However, it looks like all occurences of method in strings were also changed to getMethod.

This broke the METHOD constant in Spdy2HttpNames et al

I think the refactoring also accidentally changed a bunch of javadocs which shouldn't have been changed.


Issue comments:
@justinsb thanks again for your fix.. I did adjust it a bit and merged it + deployed it to the snapshot repository.

Could you please test with the snapshot: 
https://oss.sonatype.org/content/repositories/snapshots/io/netty/netty-all/4.0.0.Beta3-SNAPSHOT/netty-all-4.0.0.Beta3-20130228.104616-2.jar

Thanks; works.  The commitset did "break" a lot of the JavaDocs etc, but that's merely a cosmetic issue.

This issue has been fixed by #1102.


Pull request review comments:
This change doesn't look right... Can you explain ?

This change is not necessary..

Obviously you know better than me, but here's my understanding...

In Netty 4, all HTTP transfers are "chunked" in the Netty 3 sense.  They all comprise a HttpResponse, 0 or more HttpContent messages, then HttpLastContent.

We're trying to tell SPDY if it's the final frame of the stream; the correct way to do that is to check for instanceof HttpLastContent.  That's done in the encode() function (which calls this function).

(There is now a potentially missed optimization, in that we'll always send a SPDY data frame, even if it's empty, but I'm focusing on getting things working again first!)

I think we can probably delete the call to release() entirely, but without the change I get this error thrown when the SPDY connection is closed:

io.netty.channel.NoSuchBufferException: the handler 'spdySessionHandler' has no inbound byte buffer; it does not implement ChannelInboundByteHandler.
    at io.netty.channel.DefaultChannelHandlerContext.inboundByteBuffer(DefaultChannelHandlerContext.java:450)
    at io.netty.handler.codec.spdy.SpdySessionHandler.freeInboundBuffer(SpdySessionHandler.java:95)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer0(DefaultChannelHandlerContext.java:1473)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer(DefaultChannelHandlerContext.java:1456)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer0(DefaultChannelHandlerContext.java:1483)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer(DefaultChannelHandlerContext.java:1456)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer0(DefaultChannelHandlerContext.java:1483)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer(DefaultChannelHandlerContext.java:1456)
    at io.netty.channel.DefaultChannelHandlerContext.invokeFreeInboundBuffer0(DefaultChannelHandlerContext.java:1483)
    at io.netty.channel.DefaultChannelHandlerContext.access$3100(DefaultChannelHandlerContext.java:38)
    at io.netty.channel.DefaultChannelHandlerContext$21.run(DefaultChannelHandlerContext.java:1462)
    at io.netty.channel.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:259)
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:305)
    at io.netty.channel.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:110)
    at java.lang.Thread.run(Thread.java:722)

I have  used this change and my spdyserver  (using netty4)  worked !  But  without this change  my spdyserver did not responded the request ,there is no SYN_REPLY. 

char1st: Glad it works for someone else as well!

normanmaurer: ...or if you're asking why currentStreamId wasn't getting set before, if the reply wasn't transfer-encoding chunked, then I don't know.  I think that was a bug?

ok know why.. it must be ctx.inboundMessageBuffer().release() ... I will incoperate this into the fix of you..


Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHeaders.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHeaders.java
index 7afa1555902..6ab424cf22a 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHeaders.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHeaders.java
@@ -108,9 +108,9 @@ public static final class HttpNames {
          */
         public static final String HOST = ":host";
         /**
-         * {@code ":getMethod"}
+         * {@code ":method"}
          */
-        public static final String METHOD = ":getMethod";
+        public static final String METHOD = ":method";
         /**
          * {@code ":path"}
          */
@@ -120,9 +120,9 @@ public static final class HttpNames {
          */
         public static final String SCHEME = ":scheme";
         /**
-         * {@code ":getStatus"}
+         * {@code ":status"}
          */
-        public static final String STATUS = ":getStatus";
+        public static final String STATUS = ":status";
         /**
          * {@code ":version"}
          */
@@ -136,17 +136,17 @@ private HttpNames() { }
      */
     public static final class Spdy2HttpNames {
         /**
-         * {@code "getMethod"}
+         * {@code "method"}
          */
-        public static final String METHOD = "getMethod";
+        public static final String METHOD = "method";
         /**
          * {@code "scheme"}
          */
         public static final String SCHEME = "scheme";
         /**
-         * {@code "getStatus"}
+         * {@code "status"}
          */
-        public static final String STATUS = "getStatus";
+        public static final String STATUS = "status";
         /**
          * {@code "url"}
          */
@@ -231,7 +231,7 @@ public static void setHost(SpdyHeaderBlock block, String host) {
     }
 
     /**
-     * Removes the HTTP getMethod header.
+     * Removes the HTTP method header.
      */
     public static void removeMethod(int spdyVersion, SpdyHeaderBlock block) {
         if (spdyVersion < 3) {
@@ -242,7 +242,7 @@ public static void removeMethod(int spdyVersion, SpdyHeaderBlock block) {
     }
 
     /**
-     * Returns the {@link HttpMethod} represented by the HTTP getMethod header.
+     * Returns the {@link HttpMethod} represented by the HTTP method header.
      */
     public static HttpMethod getMethod(int spdyVersion, SpdyHeaderBlock block) {
         try {
@@ -257,7 +257,7 @@ public static HttpMethod getMethod(int spdyVersion, SpdyHeaderBlock block) {
     }
 
     /**
-     * Sets the HTTP getMethod header.
+     * Sets the HTTP method header.
      */
     public static void setMethod(int spdyVersion, SpdyHeaderBlock block, HttpMethod method) {
         if (spdyVersion < 3) {
@@ -301,7 +301,7 @@ public static void setScheme(int spdyVersion, SpdyHeaderBlock block, String sche
     }
 
     /**
-     * Removes the HTTP response getStatus header.
+     * Removes the HTTP response status header.
      */
     public static void removeStatus(int spdyVersion, SpdyHeaderBlock block) {
         if (spdyVersion < 3) {
@@ -312,7 +312,7 @@ public static void removeStatus(int spdyVersion, SpdyHeaderBlock block) {
     }
 
     /**
-     * Returns the {@link HttpResponseStatus} represented by the HTTP response getStatus header.
+     * Returns the {@link HttpResponseStatus} represented by the HTTP response status header.
      */
     public static HttpResponseStatus getStatus(int spdyVersion, SpdyHeaderBlock block) {
         try {
@@ -341,7 +341,7 @@ public static HttpResponseStatus getStatus(int spdyVersion, SpdyHeaderBlock bloc
     }
 
     /**
-     * Sets the HTTP response getStatus header.
+     * Sets the HTTP response status header.
      */
     public static void setStatus(int spdyVersion, SpdyHeaderBlock block, HttpResponseStatus status) {
         if (spdyVersion < 3) {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
index 28d7acfecdd..6a00be5b823 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyHttpEncoder.java
@@ -142,11 +142,16 @@ public SpdyHttpEncoder(int version) {
     protected Object encode(ChannelHandlerContext ctx, HttpObject msg) throws Exception {
 
         List<Object> out = new ArrayList<Object>();
+
+        boolean valid = false;
+
         if (msg instanceof HttpRequest) {
 
             HttpRequest httpRequest = (HttpRequest) msg;
             SpdySynStreamFrame spdySynStreamFrame = createSynStreamFrame(httpRequest);
             out.add(spdySynStreamFrame);
+
+            valid = true;
         }
         if (msg instanceof HttpResponse) {
 
@@ -158,6 +163,8 @@ protected Object encode(ChannelHandlerContext ctx, HttpObject msg) throws Except
                 SpdySynReplyFrame spdySynReplyFrame = createSynReplyFrame(httpResponse);
                 out.add(spdySynReplyFrame);
             }
+
+            valid = true;
         }
         if (msg instanceof HttpContent) {
 
@@ -185,7 +192,11 @@ protected Object encode(ChannelHandlerContext ctx, HttpObject msg) throws Except
             } else {
                 out.add(spdyDataFrame);
             }
-        } else {
+
+            valid = true;
+        }
+
+        if (!valid) {
             throw new UnsupportedMessageTypeException(msg);
         }
 
@@ -255,8 +266,6 @@ private SpdySynStreamFrame createSynStreamFrame(HttpMessage httpMessage)
 
     private SpdySynReplyFrame createSynReplyFrame(HttpResponse httpResponse)
             throws Exception {
-        boolean chunked = HttpHeaders.isTransferEncodingChunked(httpResponse);
-
         // Get the Stream-ID from the headers
         int streamID = SpdyHttpHeaders.getStreamId(httpResponse);
         SpdyHttpHeaders.removeStreamId(httpResponse);
@@ -279,10 +288,8 @@ private SpdySynReplyFrame createSynReplyFrame(HttpResponse httpResponse)
             spdySynReplyFrame.headers().add(entry.getKey(), entry.getValue());
         }
 
-        if (chunked) {
-            currentStreamId = streamID;
-            spdySynReplyFrame.setLast(false);
-        }
+        currentStreamId = streamID;
+        spdySynReplyFrame.setLast(false);
 
         return spdySynReplyFrame;
     }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
index 52e59a57c10..a57437c6e2a 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
@@ -92,7 +92,9 @@ public MessageBuf<Object> newInboundBuffer(ChannelHandlerContext ctx) throws Exc
 
     @Override
     public void freeInboundBuffer(ChannelHandlerContext ctx) throws Exception {
-        ctx.inboundByteBuffer().release();
+        if (ctx.hasInboundByteBuffer()) {
+            ctx.inboundByteBuffer().release();
+        }
     }
 
     @Override


Test patch: