From 2cf4ee93221e2ad18ca89b5ec95008c0292b8d2d Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 10 Dec 2014 21:10:26 -0500 Subject: [PATCH] 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. --- .../handler/codec/http2/DefaultHttp2Connection.java | 10 ++++------ .../codec/http2/DefaultHttp2FrameReader.java | 4 ++-- .../codec/http2/DefaultHttp2HeadersDecoder.java | 8 +++++--- .../codec/http2/DefaultHttp2HeadersEncoder.java | 9 +++++---- .../http2/DefaultHttp2RemoteFlowController.java | 9 ++++----- .../http2/DelegatingDecompressorFrameListener.java | 6 ++---- .../netty/handler/codec/http2/Http2CodecUtil.java | 2 -- .../handler/codec/http2/Http2ConnectionHandler.java | 4 ++-- .../netty/handler/codec/http2/Http2Exception.java | 9 ++------- .../codec/http2/Http2ServerUpgradeCodec.java | 3 +-- .../java/io/netty/handler/codec/http2/HttpUtil.java | 4 ++-- .../http2/InboundHttp2ToHttpPriorityAdapter.java | 2 +- .../codec/http2/DefaultHttp2ConnectionTest.java | 13 +++++++------ 13 files changed, 37 insertions(+), 46 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java index 41a49b74e7..9dd2245a7c 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Connection.java +++ b/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()) { - child.takeChild(grandchild, false, events); - } + for (DefaultStream grandchild : removeAllChildren().values()) { + 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); } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java index 32b2fceeea..d3ee623ed1 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java +++ b/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); diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java index 10c37ce60c..70f62dc4e1 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java +++ b/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()); } } } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java index e7c555c882..a1f950b1b0 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersEncoder.java +++ b/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) { - throw connectionError(COMPRESSION_ERROR, "Failed encoding headers block: %s", - e.getMessage()); + } catch (Http2Exception e) { + throw e; + } 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()); } } } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java index dfc5483c3e..5cab546ad6 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2RemoteFlowController.java +++ b/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(2); } @@ -590,9 +590,8 @@ public class DefaultHttp2RemoteFlowController implements Http2RemoteFlowControll try { connectionState().incrementStreamWindow(-bytesToWrite); incrementStreamWindow(-bytesToWrite); - } catch (Http2Exception e) { - // Should never get here since we're decrementing. - throw new AssertionError("Invalid window state when writing frame: " + e.getMessage()); + } catch (Http2Exception e) { // Should never get here since we're decrementing. + throw new RuntimeException("Invalid window state when writing frame: " + e.getMessage(), e); } 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() { diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DelegatingDecompressorFrameListener.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DelegatingDecompressorFrameListener.java index 2477fd9e20..060e327932 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DelegatingDecompressorFrameListener.java +++ b/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) { diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java index 55168a0c32..44d734dd20 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java +++ b/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; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java index 7ac109aeb3..f992f2a6bb 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java +++ b/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; } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java index 15f52989ee..7f69fe3a58 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Exception.java +++ b/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; } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java index d5d8108608..822dce42a5 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ServerUpgradeCodec.java +++ b/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) { diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java index 963beb81b7..1877a236dc 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HttpUtil.java +++ b/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) { - throw connectionError(PROTOCOL_ERROR, + } catch (Throwable t) { + throw connectionError(PROTOCOL_ERROR, t, "Unrecognized HTTP status code '%s' encountered in translation to HTTP/1.x", status); } return result; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java index 23019ad1ee..38c240d7dd 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttp2ToHttpPriorityAdapter.java +++ b/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(); } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java index e563785a3a..3dbe262b11 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionTest.java +++ b/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 expectedArg1, List expectedArg2) { - assertTrue(expectedArg1.size() == expectedArg2.size()); + assertSame(expectedArg1.size(), expectedArg2.size()); ArgumentCaptor arg1Captor = ArgumentCaptor.forClass(Http2Stream.class); ArgumentCaptor arg2Captor = ArgumentCaptor.forClass(Http2Stream.class); verify(clientListener, times(expectedArg1.size())).priorityTreeParentChanging(arg1Captor.capture(), arg2Captor.capture()); List capturedArg1 = arg1Captor.getAllValues(); List capturedArg2 = arg2Captor.getAllValues(); - assertTrue(capturedArg1.size() == capturedArg2.size()); - assertTrue(capturedArg1.size() == expectedArg1.size()); + assertSame(capturedArg1.size(), capturedArg2.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 expectedArg1, List expectedArg2) { - assertTrue(expectedArg1.size() == expectedArg2.size()); + assertSame(expectedArg1.size(), expectedArg2.size()); ArgumentCaptor arg1Captor = ArgumentCaptor.forClass(Http2Stream.class); ArgumentCaptor arg2Captor = ArgumentCaptor.forClass(Http2Stream.class); verify(clientListener, times(expectedArg1.size())).priorityTreeParentChanged(arg1Captor.capture(), arg2Captor.capture()); List capturedArg1 = arg1Captor.getAllValues(); List capturedArg2 = arg2Captor.getAllValues(); - assertTrue(capturedArg1.size() == capturedArg2.size()); - assertTrue(capturedArg1.size() == expectedArg1.size()); + assertSame(capturedArg1.size(), capturedArg2.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));