From ab74dccd235dcd10bd14685c7095a6a647b4a037 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 25 Mar 2015 16:29:14 -0700 Subject: [PATCH] Http/2 Priority on CLOSED stream Motivation: The encoder/decoder currently do not handle streams which have previously existed but no longer exist because they were closed. The specification requires supporting this. Modifications: - encoder/decoder should tolerate the frame or the dependent frame not existing in the streams map due to the fact that it may have previously existed. Result: encoder/decoder are more compliant with the specification. --- .../codec/http2/DefaultHttp2Connection.java | 24 +++++----- .../http2/DefaultHttp2ConnectionDecoder.java | 22 +++++---- .../http2/DefaultHttp2ConnectionEncoder.java | 10 ++++- .../handler/codec/http2/Http2Exception.java | 31 +++++++++++++ .../codec/http2/Http2FrameListener.java | 40 +++++++++-------- .../DefaultHttp2ConnectionDecoderTest.java | 35 +++++++++++++++ .../DefaultHttp2ConnectionEncoderTest.java | 45 ++++++++++++++++--- 7 files changed, 161 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 f87e098d27a..408f17dc4e6 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 @@ -22,6 +22,7 @@ import static io.netty.handler.codec.http2.Http2CodecUtil.immediateRemovalPolicy; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.netty.handler.codec.http2.Http2Error.REFUSED_STREAM; +import static io.netty.handler.codec.http2.Http2Exception.closedStreamError; import static io.netty.handler.codec.http2.Http2Exception.connectionError; import static io.netty.handler.codec.http2.Http2Exception.streamError; import static io.netty.handler.codec.http2.Http2Stream.State.CLOSED; @@ -689,8 +690,7 @@ private final class DefaultEndpoint implements En @Override public int nextStreamId() { - // For manually created client-side streams, 1 is reserved for HTTP upgrade, so - // start at 3. + // For manually created client-side streams, 1 is reserved for HTTP upgrade, so start at 3. return nextStreamId > 1 ? nextStreamId : nextStreamId + 2; } @@ -836,24 +836,22 @@ private void checkNewStreamAllowed(int streamId) throws Http2Exception { if (isGoAway()) { throw connectionError(PROTOCOL_ERROR, "Cannot create a stream since the connection is going away"); } - verifyStreamId(streamId); - if (!canCreateStream()) { - throw connectionError(REFUSED_STREAM, "Maximum streams exceeded for this endpoint."); - } - } - - private void verifyStreamId(int streamId) throws Http2Exception { if (streamId < 0) { throw new Http2NoMoreStreamIdsException(); } - if (streamId < nextStreamId) { - throw connectionError(PROTOCOL_ERROR, "Request stream %d is behind the next expected stream %d", - streamId, nextStreamId); - } if (!createdStreamId(streamId)) { throw connectionError(PROTOCOL_ERROR, "Request stream %d is not correct for %s connection", streamId, server ? "server" : "client"); } + // This check must be after all id validated checks, but before the max streams check because it may be + // recoverable to some degree for handling frames which can be sent on closed streams. + if (streamId < nextStreamId) { + throw closedStreamError(PROTOCOL_ERROR, "Request stream %d is behind the next expected stream %d", + streamId, nextStreamId); + } + if (!canCreateStream()) { + throw connectionError(REFUSED_STREAM, "Maximum streams exceeded for this endpoint."); + } } private boolean isLocal() { diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java index 5c214617042..33fbc1cf707 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java @@ -24,6 +24,7 @@ import static io.netty.util.internal.ObjectUtil.checkNotNull; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; +import io.netty.handler.codec.http2.Http2Exception.ClosedStreamCreationException; import java.util.List; @@ -374,15 +375,20 @@ public void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDe return; } - if (stream == null) { - // PRIORITY frames always identify a stream. This means that if a PRIORITY frame is the - // first frame to be received for a stream that we must create the stream. - stream = connection.remote().createStream(streamId); - } + try { + if (stream == null) { + // PRIORITY frames always identify a stream. This means that if a PRIORITY frame is the + // first frame to be received for a stream that we must create the stream. + stream = connection.remote().createStream(streamId); + } - // This call will create a stream for streamDependency if necessary. - // For this reason it must be done before notifying the listener. - stream.setPriority(streamDependency, weight, exclusive); + // This call will create a stream for streamDependency if necessary. + // For this reason it must be done before notifying the listener. + stream.setPriority(streamDependency, weight, exclusive); + } catch (ClosedStreamCreationException ignored) { + // It is possible that either the stream for this frame or the parent stream is closed. + // In this case we should ignore the exception and allow the frame to be sent. + } listener.onPriorityRead(ctx, streamId, streamDependency, weight, exclusive); } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java index 37b723a03bf..0e0bca4d808 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionEncoder.java @@ -24,6 +24,7 @@ import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelPromise; +import io.netty.handler.codec.http2.Http2Exception.ClosedStreamCreationException; import io.netty.util.ReferenceCountUtil; import java.util.ArrayDeque; @@ -238,9 +239,14 @@ public ChannelFuture writePriority(ChannelHandlerContext ctx, int streamId, int stream = connection.local().createStream(streamId); } + // The set priority operation must be done before sending the frame. The parent may not yet exist + // and the priority tree may also be modified before sending. stream.setPriority(streamDependency, weight, exclusive); - } catch (Throwable e) { - return promise.setFailure(e); + } catch (ClosedStreamCreationException ignored) { + // It is possible that either the stream for this frame or the parent stream is closed. + // In this case we should ignore the exception and allow the frame to be sent. + } catch (Throwable t) { + return promise.setFailure(t); } ChannelFuture future = frameWriter.writePriority(ctx, streamId, streamDependency, weight, exclusive, promise); 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 7f69fe3a581..3bfff8f1feb 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 @@ -72,6 +72,18 @@ public static Http2Exception connectionError(Http2Error error, Throwable cause, return new Http2Exception(error, String.format(fmt, args), cause); } + /** + * Use if an error has occurred which can not be isolated to a single stream, but instead applies + * to the entire connection. + * @param error The type of error as defined by the HTTP/2 specification. + * @param fmt String with the content and format for the additional debug data. + * @param args Objects which fit into the format defined by {@code fmt}. + * @return An exception which can be translated into a HTTP/2 error. + */ + public static Http2Exception closedStreamError(Http2Error error, String fmt, Object... args) { + return new ClosedStreamCreationException(error, String.format(fmt, args)); + } + /** * Use if an error which can be isolated to a single stream has occurred. If the {@code id} is not * {@link Http2CodecUtil#CONNECTION_STREAM_ID} then a {@link Http2Exception.StreamException} will be returned. @@ -130,6 +142,25 @@ public static int streamId(Http2Exception e) { return isStreamError(e) ? ((StreamException) e).streamId() : CONNECTION_STREAM_ID; } + /** + * Used when a stream creation attempt fails but may be because the stream was previously closed. + */ + public static final class ClosedStreamCreationException extends Http2Exception { + private static final long serialVersionUID = -1911637707391622439L; + + public ClosedStreamCreationException(Http2Error error) { + super(error); + } + + public ClosedStreamCreationException(Http2Error error, String message) { + super(error, message); + } + + public ClosedStreamCreationException(Http2Error error, String message, Throwable cause) { + super(error, message, cause); + } + } + /** * Represents an exception that can be isolated to a single stream (as opposed to the entire connection). */ diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java index ab03d9e7952..2bb9a2125fe 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java @@ -46,10 +46,10 @@ int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int paddin boolean endOfStream) throws Http2Exception; /** - * Handles an inbound HEADERS frame. + * Handles an inbound {@code HEADERS} frame. *

- * Only one of the following methods will be called for each HEADERS frame sequence. - * One will be called when the END_HEADERS flag has been received. + * Only one of the following methods will be called for each {@code HEADERS} frame sequence. + * One will be called when the {@code END_HEADERS} flag has been received. *