New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix StreamBufferingEncoder GOAWAY bug #11144
Conversation
@dapengzhang0 Please sign the ICLA: https://netty.io/wiki/developer-guide.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me, but I'm not that familiar with the H2 code, so I'd like a second opinion before merging.
@chrisvest, don't merge yet. I've been discussing with @dapengzhang0 on grpc/grpc-java#8020 (comment) . I expect we'll want to use a slightly different ordering of |
@ejona86 The different ordering is done. PTAL |
codec-http2/src/test/java/io/netty/handler/codec/http2/StreamBufferingEncoderTest.java
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the signature of cancelGoAwayStreams was changed, but it's fine.
@chrisvest, do you want to take another look? (I'm not asking you to, I'm asking if you want to.) |
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
Yeah not related to this pr
… Am 12.04.2021 um 21:54 schrieb Eric Anderson ***@***.***>:
@ejona86 commented on this pull request.
In codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java:
> @@ -106,6 +106,9 @@ public long errorCode() {
private final TreeMap<Integer, PendingStream> pendingStreams = new TreeMap<Integer, PendingStream>();
private int maxConcurrentStreams;
private boolean closed;
+ private Integer goAwayLastStreamId;
+ private long goAwayErrorCode;
+ private ByteBuf goAwayDebugData;
clone in the constructor
I don't see how that helps this PR, as a single exception can be received by many pieces of code. It seems the getter Http2GoAwayException.debugData() would need to clone. The constructor could defend itself with another clone to handle user code that creates Http2GoAwayException (which is mostly theoretical), but that wouldn't impact any of the code being changed in this PR.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer, want to take another look?
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
…ufferingEncoder.java
…ufferingEncoder.java
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
…ufferingEncoder.java
codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Outdated
Show resolved
Hide resolved
…ufferingEncoder.java
Thanks a lot |
Motivation: There is a bug in `StreamBufferingEncoder` such that when client receives GOWAY while there are pending streams due to MAX_CONCURRENT_STREAMS, we see the following error: ``` io.netty.handler.codec.http2.Http2Exception$StreamException: Maximum active streams violated for this endpoint. at io.netty.handler.codec.http2.Http2Exception.streamError(Http2Exception.java:147) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.checkNewStreamAllowed(DefaultHttp2Connection.java:896) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:748) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:668) at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders0(DefaultHttp2ConnectionEncoder.java:201) at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders(DefaultHttp2ConnectionEncoder.java:167) at io.netty.handler.codec.http2.DecoratingHttp2FrameWriter.writeHeaders(DecoratingHttp2FrameWriter.java:53) at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:153) at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:141) at io.grpc.netty.NettyClientHandler.createStreamTraced(NettyClientHandler.java:584) at io.grpc.netty.NettyClientHandler.createStream(NettyClientHandler.java:567) at io.grpc.netty.NettyClientHandler.write(NettyClientHandler.java:328) at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:717) at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:709) at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:792) at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:702) at io.netty.channel.DefaultChannelPipeline.write(DefaultChannelPipeline.java:1015) at io.netty.channel.AbstractChannel.write(AbstractChannel.java:289) at io.grpc.netty.WriteQueue$AbstractQueuedCommand.run(WriteQueue.java:213) at io.grpc.netty.WriteQueue.flush(WriteQueue.java:128) at io.grpc.netty.WriteQueue.drainNow(WriteQueue.java:114) at io.grpc.netty.NettyClientHandler.goingAway(NettyClientHandler.java:783) at io.grpc.netty.NettyClientHandler.access$300(NettyClientHandler.java:91) at io.grpc.netty.NettyClientHandler$3.onGoAwayReceived(NettyClientHandler.java:280) at io.netty.handler.codec.http2.DefaultHttp2Connection.goAwayReceived(DefaultHttp2Connection.java:236) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.onGoAwayRead0(DefaultHttp2ConnectionDecoder.java:218) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onGoAwayRead(DefaultHttp2ConnectionDecoder.java:551) at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onGoAwayRead(Http2InboundFrameLogger.java:119) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readGoAwayFrame(DefaultHttp2FrameReader.java:591) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:272) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160) at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:174) at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:378) at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357) at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1486) at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1235) at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1282) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357) at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:792) at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:475) at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:378) at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989) at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.base/java.lang.Thread.run(Unknown Source) ``` The bug should come from the way that `StreamBufferingEncoder.writeHeaders()` handles the condition `connection().goAwayReceived()`. The current behavior is to delegate to `super.writeHeaders()` and let the stream fail, but this will end up with `Http2Exception` with the message "Maximum active streams violated for this endpoint" which is horrible. See https://github.com/netty/netty/blob/e5951d46fc89db507ba7d2968d2ede26378f0b04/codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java#L152-L155 Modification: Abort new stream immediately if goaway received *and* MAX_CONCURRENT_STREAM reached in `StreamBufferingEncoder` rather than delegating to the `writeHeaders()` method of its super class. Result: In the situation when GOAWAY received as well as MAX_CONCURRENT_STREAM exceeded, the client will fail the buffered streams with `Http2Error.NO_ERROR` and message "GOAWAY received" instead of "Maximum active streams violated for this endpoint". Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation: There is a bug in `StreamBufferingEncoder` such that when client receives GOWAY while there are pending streams due to MAX_CONCURRENT_STREAMS, we see the following error: ``` io.netty.handler.codec.http2.Http2Exception$StreamException: Maximum active streams violated for this endpoint. at io.netty.handler.codec.http2.Http2Exception.streamError(Http2Exception.java:147) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.checkNewStreamAllowed(DefaultHttp2Connection.java:896) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:748) at io.netty.handler.codec.http2.DefaultHttp2Connection$DefaultEndpoint.createStream(DefaultHttp2Connection.java:668) at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders0(DefaultHttp2ConnectionEncoder.java:201) at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.writeHeaders(DefaultHttp2ConnectionEncoder.java:167) at io.netty.handler.codec.http2.DecoratingHttp2FrameWriter.writeHeaders(DecoratingHttp2FrameWriter.java:53) at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:153) at io.netty.handler.codec.http2.StreamBufferingEncoder.writeHeaders(StreamBufferingEncoder.java:141) at io.grpc.netty.NettyClientHandler.createStreamTraced(NettyClientHandler.java:584) at io.grpc.netty.NettyClientHandler.createStream(NettyClientHandler.java:567) at io.grpc.netty.NettyClientHandler.write(NettyClientHandler.java:328) at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:717) at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:709) at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:792) at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:702) at io.netty.channel.DefaultChannelPipeline.write(DefaultChannelPipeline.java:1015) at io.netty.channel.AbstractChannel.write(AbstractChannel.java:289) at io.grpc.netty.WriteQueue$AbstractQueuedCommand.run(WriteQueue.java:213) at io.grpc.netty.WriteQueue.flush(WriteQueue.java:128) at io.grpc.netty.WriteQueue.drainNow(WriteQueue.java:114) at io.grpc.netty.NettyClientHandler.goingAway(NettyClientHandler.java:783) at io.grpc.netty.NettyClientHandler.access$300(NettyClientHandler.java:91) at io.grpc.netty.NettyClientHandler$3.onGoAwayReceived(NettyClientHandler.java:280) at io.netty.handler.codec.http2.DefaultHttp2Connection.goAwayReceived(DefaultHttp2Connection.java:236) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.onGoAwayRead0(DefaultHttp2ConnectionDecoder.java:218) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onGoAwayRead(DefaultHttp2ConnectionDecoder.java:551) at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onGoAwayRead(Http2InboundFrameLogger.java:119) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readGoAwayFrame(DefaultHttp2FrameReader.java:591) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:272) at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160) at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41) at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:174) at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:378) at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357) at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1486) at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1235) at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1282) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:498) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:437) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357) at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365) at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:792) at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:475) at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:378) at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989) at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.base/java.lang.Thread.run(Unknown Source) ``` The bug should come from the way that `StreamBufferingEncoder.writeHeaders()` handles the condition `connection().goAwayReceived()`. The current behavior is to delegate to `super.writeHeaders()` and let the stream fail, but this will end up with `Http2Exception` with the message "Maximum active streams violated for this endpoint" which is horrible. See https://github.com/netty/netty/blob/e5951d46fc89db507ba7d2968d2ede26378f0b04/codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java#L152-L155 Modification: Abort new stream immediately if goaway received *and* MAX_CONCURRENT_STREAM reached in `StreamBufferingEncoder` rather than delegating to the `writeHeaders()` method of its super class. Result: In the situation when GOAWAY received as well as MAX_CONCURRENT_STREAM exceeded, the client will fail the buffered streams with `Http2Error.NO_ERROR` and message "GOAWAY received" instead of "Maximum active streams violated for this endpoint". Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
There is a bug in
StreamBufferingEncoder
such that when client receives GOWAY while there are pending streams due to MAX_CONCURRENT_STREAMS, we see the following error:The bug should come from the way that
StreamBufferingEncoder.writeHeaders()
handles the conditionconnection().goAwayReceived()
. The current behavior is to delegate tosuper.writeHeaders()
and let the stream fail, but this will end up withHttp2Exception
with the message "Maximum active streams violated for this endpoint" which is horrible. Seenetty/codec-http2/src/main/java/io/netty/handler/codec/http2/StreamBufferingEncoder.java
Lines 152 to 155 in e5951d4
Modification:
Abort new stream immediately if goaway received and MAX_CONCURRENT_STREAM reached in
StreamBufferingEncoder
rather than delegating to thewriteHeaders()
method of its super class.Result:
In the situation when GOAWAY received as well as MAX_CONCURRENT_STREAM exceeded, the client will fail the buffered streams with
Http2Error.NO_ERROR
and message "GOAWAY received" instead of "Maximum active streams violated for this endpoint".