HTTP/2 Sonar Bugs

Motivation:
Clinker provides a Sonar tool which detects potential bugs or problems in the code.  These problems were reported here http://clinker.netty.io/sonar/drilldown/issues/io.netty:netty-parent:master?

Modifications:
Make the recommended changes as reported by Sonar

Result:
Better or more standard code.  Less Sonar problem reports for HTTP/2 codec.

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java

@@ -513,14 +513,12 @@ public class DefaultHttp2Connection implements Http2Connection {
             notifyParentChanging(child, this);
             child.parent = this;
 
-            if (exclusive) {
+            if (exclusive && !children.isEmpty()) {
                 // If it was requested that this child be the exclusive dependency of this node,
                 // move any previous children to the child node, becoming grand children
                 // of this node.
-                if (!children.isEmpty()) {
+                for (DefaultStream grandchild : removeAllChildren().values()) {
-                    for (DefaultStream grandchild : removeAllChildren().values()) {
+                    child.takeChild(grandchild, false, events);
-                        child.takeChild(grandchild, false, events);
-                    }
                 }
             }
 
@@ -676,7 +674,7 @@ public class DefaultHttp2Connection implements Http2Connection {
      * Stream class representing the connection, itself.
      */
     private final class ConnectionStream extends DefaultStream {
-        private ConnectionStream() {
+        ConnectionStream() {
             super(CONNECTION_STREAM_ID);
         }
 

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java

@@ -403,7 +403,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
         // is present in the headers frame.
         if (flags.priorityPresent()) {
             long word1 = payload.readUnsignedInt();
-            final boolean exclusive = (word1 & 0x80000000L) > 0;
+            final boolean exclusive = (word1 & 0x80000000L) != 0;
             final int streamDependency = (int) (word1 & 0x7FFFFFFFL);
             final short weight = (short) (payload.readUnsignedByte() + 1);
             final ByteBuf fragment = payload.readSlice(payload.readableBytes() - padding);
@@ -462,7 +462,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
     private void readPriorityFrame(ChannelHandlerContext ctx, ByteBuf payload,
             Http2FrameListener listener) throws Http2Exception {
         long word1 = payload.readUnsignedInt();
-        boolean exclusive = (word1 & 0x80000000L) > 0;
+        boolean exclusive = (word1 & 0x80000000L) != 0;
         int streamDependency = (int) (word1 & 0x7FFFFFFFL);
         short weight = (short) (payload.readUnsignedByte() + 1);
         listener.onPriorityRead(ctx, streamId, streamDependency, weight, exclusive);

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java

@@ -79,17 +79,19 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea
 
             return headers;
         } catch (IOException e) {
-            throw new Http2Exception(COMPRESSION_ERROR, e.getMessage());
+            throw connectionError(COMPRESSION_ERROR, e, e.getMessage());
+        } catch (Http2Exception e) {
+            throw e;
         } catch (Throwable e) {
             // Default handler for any other types of errors that may have occurred. For example,
             // the the Header builder throws IllegalArgumentException if the key or value was invalid
             // for any reason (e.g. the key was an invalid pseudo-header).
-            throw new Http2Exception(PROTOCOL_ERROR, e.getMessage(), e);
+            throw connectionError(PROTOCOL_ERROR, e, e.getMessage());
         } finally {
             try {
                 in.close();
             } catch (IOException e) {
-                throw new Http2Exception(INTERNAL_ERROR, e.getMessage(), e);
+                throw connectionError(INTERNAL_ERROR, e, e.getMessage());
             }
         }
     }

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java

@@ -87,14 +87,15 @@ public class DefaultHttp2HeadersEncoder implements Http2HeadersEncoder, Http2Hea
                     return true;
                 }
             });
-        } catch (Exception e) {
+        } catch (Http2Exception e) {
-            throw connectionError(COMPRESSION_ERROR, "Failed encoding headers block: %s",
+            throw e;
-                            e.getMessage());
+        } catch (Throwable t) {
+            throw connectionError(COMPRESSION_ERROR, t, "Failed encoding headers block: %s", t.getMessage());
         } finally {
             try {
                 stream.close();
             } catch (IOException e) {
-                throw new Http2Exception(INTERNAL_ERROR, e.getMessage(), e);
+                throw connectionError(INTERNAL_ERROR, e, e.getMessage());
             }
         }
     }

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java

@@ -361,7 +361,7 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
         private int allocated;
         private ChannelFuture lastNewFrame;
 
-        private FlowState(Http2Stream stream) {
+        FlowState(Http2Stream stream) {
             this.stream = stream;
             pendingWriteQueue = new ArrayDeque<Frame>(2);
         }
@@ -590,9 +590,8 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
                         try {
                             connectionState().incrementStreamWindow(-bytesToWrite);
                             incrementStreamWindow(-bytesToWrite);
-                        } catch (Http2Exception e) {
+                        } catch (Http2Exception e) { // Should never get here since we're decrementing.
-                            // Should never get here since we're decrementing.
+                            throw new RuntimeException("Invalid window state when writing frame: " + e.getMessage(), e);
-                            throw new AssertionError("Invalid window state when writing frame: " + e.getMessage());
                         }
                         frameWriter.writeData(ctx, stream.id(), data, padding, endStream, promise);
                         frameSent = true;
@@ -662,7 +661,7 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll
     /**
      * Lightweight promise aggregator.
      */
-    private class SimplePromiseAggregator {
+    private static final class SimplePromiseAggregator {
         final ChannelPromise promise;
         final AtomicInteger awaiting = new AtomicInteger();
         final ChannelFutureListener listener = new ChannelFutureListener() {

codec-http2/src/main/java/io/netty/handler/codec/http2/DelegatingDecompressorFrameListener.java

@@ -118,9 +118,7 @@ public class DelegatingDecompressorFrameListener extends Http2FrameListenerDecor
                         buf = nextBuf;
                     }
                 } finally {
-                    if (buf != null) {
+                    buf.release();
-                        buf.release();
-                    }
                 }
             }
             decompressor.incrementProcessedBytes(processedBytes);
@@ -290,7 +288,7 @@ public class DelegatingDecompressorFrameListener extends Http2FrameListenerDecor
     /**
      * A decorator around the local flow controller that converts consumed bytes from uncompressed to compressed.
      */
-    private final class ConsumedBytesConverter implements Http2LocalFlowController {
+    private static final class ConsumedBytesConverter implements Http2LocalFlowController {
         private final Http2LocalFlowController flowController;
 
         ConsumedBytesConverter(Http2LocalFlowController flowController) {

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java

@@ -19,8 +19,6 @@ import static io.netty.util.CharsetUtil.UTF_8;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
-import io.netty.channel.ChannelHandler;
-import io.netty.channel.ChannelHandlerAdapter;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPromise;
 import io.netty.handler.codec.http2.Http2StreamRemovalPolicy.Action;

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

@@ -373,7 +373,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
         try {
             // Read the remaining of the client preface string if we haven't already.
             // If this is a client endpoint, always returns true.
-            if (!readClientPrefaceString(ctx, in)) {
+            if (!readClientPrefaceString(in)) {
                 // Still processing the client preface.
                 return;
             }
@@ -422,7 +422,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
      * @return {@code true} if processing of the client preface string is complete. Since client preface strings can
      *         only be received by servers, returns true immediately for client endpoints.
      */
-    private boolean readClientPrefaceString(ChannelHandlerContext ctx, ByteBuf in) throws Http2Exception {
+    private boolean readClientPrefaceString(ByteBuf in) throws Http2Exception {
         if (clientPrefaceString == null) {
             return true;
         }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java

@@ -137,21 +137,16 @@ public class Http2Exception extends Exception {
         private static final long serialVersionUID = 462766352505067095L;
         private final int streamId;
 
-        private StreamException(int streamId, Http2Error error, String message) {
+        StreamException(int streamId, Http2Error error, String message) {
             super(error, message);
             this.streamId = streamId;
         }
 
-        private StreamException(int streamId, Http2Error error, String message, Throwable cause) {
+        StreamException(int streamId, Http2Error error, String message, Throwable cause) {
             super(error, message, cause);
             this.streamId = streamId;
         }
 
-        private StreamException(int streamId, Http2Error error) {
-            super(error);
-            this.streamId = streamId;
-        }
-
         public int streamId() {
             return streamId;
         }

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java

@@ -47,7 +47,6 @@ public class Http2ServerUpgradeCodec implements HttpServerUpgradeHandler.Upgrade
     private final String handlerName;
     private final Http2ConnectionHandler connectionHandler;
     private final Http2FrameReader frameReader;
-    private Http2Settings settings;
 
     /**
      * Creates the codec using a default name for the connection handler when adding to the
@@ -92,7 +91,7 @@ public class Http2ServerUpgradeCodec implements HttpServerUpgradeHandler.Upgrade
                 throw new IllegalArgumentException("There must be 1 and only 1 "
                         + HTTP_UPGRADE_SETTINGS_HEADER + " header.");
             }
-            settings = decodeSettingsHeader(ctx, upgradeHeaders.get(0));
+            Http2Settings settings = decodeSettingsHeader(ctx, upgradeHeaders.get(0));
             connectionHandler.onHttpServerUpgrade(settings);
             // Everything looks good, no need to modify the response.
         } catch (Throwable e) {

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java

@@ -175,8 +175,8 @@ public final class HttpUtil {
             }
         } catch (Http2Exception e) {
             throw e;
-        } catch (Exception ignored) {
+        } catch (Throwable t) {
-            throw connectionError(PROTOCOL_ERROR,
+            throw connectionError(PROTOCOL_ERROR, t,
                             "Unrecognized HTTP status code '%s' encountered in translation to HTTP/1.x", status);
         }
         return result;

codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java

@@ -62,7 +62,7 @@ public final class InboundHttp2ToHttpPriorityAdapter extends InboundHttp2ToHttpA
         }
     }
 
-    private InboundHttp2ToHttpPriorityAdapter(Builder builder) {
+    InboundHttp2ToHttpPriorityAdapter(Builder builder) {
         super(builder);
         outOfMessageFlowHeaders = new IntObjectHashMap<HttpHeaders>();
     }

codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java

@@ -20,6 +20,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertSame;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyShort;
 import static org.mockito.Matchers.eq;
@@ -532,15 +533,15 @@ public class DefaultHttp2ConnectionTest {
     }
 
     private void verifyParentChanging(List<Http2Stream> expectedArg1, List<Http2Stream> expectedArg2) {
-        assertTrue(expectedArg1.size() == expectedArg2.size());
+        assertSame(expectedArg1.size(), expectedArg2.size());
         ArgumentCaptor<Http2Stream> arg1Captor = ArgumentCaptor.forClass(Http2Stream.class);
         ArgumentCaptor<Http2Stream> arg2Captor = ArgumentCaptor.forClass(Http2Stream.class);
         verify(clientListener, times(expectedArg1.size())).priorityTreeParentChanging(arg1Captor.capture(),
                         arg2Captor.capture());
         List<Http2Stream> capturedArg1 = arg1Captor.getAllValues();
         List<Http2Stream> capturedArg2 = arg2Captor.getAllValues();
-        assertTrue(capturedArg1.size() == capturedArg2.size());
+        assertSame(capturedArg1.size(), capturedArg2.size());
-        assertTrue(capturedArg1.size() == expectedArg1.size());
+        assertSame(capturedArg1.size(), expectedArg1.size());
         for (int i = 0; i < capturedArg1.size(); ++i) {
             assertEquals(expectedArg1.get(i), capturedArg1.get(i));
             assertEquals(expectedArg2.get(i), capturedArg2.get(i));
@@ -548,15 +549,15 @@ public class DefaultHttp2ConnectionTest {
     }
 
     private void verifyParentsChanged(List<Http2Stream> expectedArg1, List<Http2Stream> expectedArg2) {
-        assertTrue(expectedArg1.size() == expectedArg2.size());
+        assertSame(expectedArg1.size(), expectedArg2.size());
         ArgumentCaptor<Http2Stream> arg1Captor = ArgumentCaptor.forClass(Http2Stream.class);
         ArgumentCaptor<Http2Stream> arg2Captor = ArgumentCaptor.forClass(Http2Stream.class);
         verify(clientListener, times(expectedArg1.size())).priorityTreeParentChanged(arg1Captor.capture(),
                         arg2Captor.capture());
         List<Http2Stream> capturedArg1 = arg1Captor.getAllValues();
         List<Http2Stream> capturedArg2 = arg2Captor.getAllValues();
-        assertTrue(capturedArg1.size() == capturedArg2.size());
+        assertSame(capturedArg1.size(), capturedArg2.size());
-        assertTrue(capturedArg1.size() == expectedArg1.size());
+        assertSame(capturedArg1.size(), expectedArg1.size());
         for (int i = 0; i < capturedArg1.size(); ++i) {
             assertEquals(expectedArg1.get(i), capturedArg1.get(i));
             assertEquals(expectedArg2.get(i), capturedArg2.get(i));