Skip to content
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

no WINDOW_UPDATE sent due to uncaught error in promise #9663

Closed
ti2ger92 opened this issue Oct 14, 2019 · 12 comments · Fixed by #9664
Closed

no WINDOW_UPDATE sent due to uncaught error in promise #9663

ti2ger92 opened this issue Oct 14, 2019 · 12 comments · Fixed by #9664
Milestone

Comments

@ti2ger92
Copy link

Expected behavior

Server sends a WINDOW_UPDATE after the window is completed.

Actual behavior

Server throws exceptions from:

after Delta grows perpetually, never being reset, and throws this exception upstream via a promise to which nothing listening.

Steps to reproduce

Run the test class in this gradle project.

Minimal yet complete reproducer code (or URL to code)

https://gist.github.com/ti2ger92/18c9c4439192d7e88996dbe928e3c300

Netty version

4.1.42

JVM version (e.g. java -version)

1.8.0_202, x86_64: "AdoptOpenJDK 8"

OS version (e.g. uname -a)

Darwin f01898596f5f.ant.amazon.com 17.7.0 Darwin Kernel Version 17.7.0: Sun Jun 2 20:31:42 PDT 2019; root:xnu-4570.71.46~1/RELEASE_X86_64 x86_64

@normanmaurer
Copy link
Member

@carl-mastrangelo @bryce-anderson can you have a look ?

normanmaurer added a commit that referenced this issue Oct 14, 2019
…multiplexed channel

Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663
@normanmaurer
Copy link
Member

@rhernandez35
Copy link

rhernandez35 commented Oct 14, 2019

@normanmaurer I do agree that closing the channel is the correct response to this exception. However, the larger issue is that we see this exception thrown many times per connection, even for requests that succeed. It seems like there's something wrong with how the flow control windows tracked in DefaultHttp2LocalFlowController and AbstractHttp2StreamChannel are calculated.

Specifically, AbstractHttp2StreamChannel's flowControlledBytes counter seems to get out sync with DefaultHttp2LocalFlowController's processedWindow, causing it to throw when attempting to flush the window update frame on read. As far as I can tell, our client isn't misbehaving.

@normanmaurer
Copy link
Member

I will try to investigate but I am not sure when exactly I will have the time yet :(

@normanmaurer
Copy link
Member

@rhernandez35 @ti2ger92 funny enough my PR did have the side-effect of what seems to be the same issue you reported here:

io.netty.handler.codec.http2.Http2Exception: Attempting to return too many bytes for stream 0
	at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:103)
	at io.netty.handler.codec.http2.Http2Exception.streamError(Http2Exception.java:146)
	at io.netty.handler.codec.http2.DefaultHttp2LocalFlowController$DefaultState.returnProcessedBytes(DefaultHttp2LocalFlowController.java:431)
	at io.netty.handler.codec.http2.DefaultHttp2LocalFlowController$DefaultState.consumeBytes(DefaultHttp2LocalFlowController.java:440)
	at io.netty.handler.codec.http2.DefaultHttp2LocalFlowController.consumeBytes(DefaultHttp2LocalFlowController.java:189)
	at io.netty.handler.codec.http2.Http2FrameCodec.consumeBytes(Http2FrameCodec.java:364)
	at io.netty.handler.codec.http2.Http2FrameCodec.write(Http2FrameCodec.java:307)
	at io.netty.handler.codec.http2.Http2MultiplexCodec$Http2MultiplexCodecStreamChannel.write0(Http2MultiplexCodec.java:313)
	at io.netty.handler.codec.http2.AbstractHttp2StreamChannel$Http2ChannelUnsafe.updateLocalWindowIfNeeded(AbstractHttp2StreamChannel.java:837)
	at io.netty.handler.codec.http2.AbstractHttp2StreamChannel$Http2ChannelUnsafe.beginRead(AbstractHttp2StreamChannel.java:777)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.read(DefaultChannelPipeline.java:1374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeRead(AbstractChannelHandlerContext.java:685)
	at io.netty.channel.AbstractChannelHandlerContext.read(AbstractChannelHandlerContext.java:670)
	at io.netty.handler.codec.http2.Http2MultiplexTest$24.channelRead(Http2MultiplexTest.java:1138)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1422)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:931)
	at io.netty.handler.codec.http2.AbstractHttp2StreamChannel$Http2ChannelUnsafe.doRead0(AbstractHttp2StreamChannel.java:872)
	at io.netty.handler.codec.http2.AbstractHttp2StreamChannel.fireChildRead(AbstractHttp2StreamChannel.java:550)
	at io.netty.handler.codec.http2.Http2MultiplexCodec.onHttp2Frame(Http2MultiplexCodec.java:142)
	at io.netty.handler.codec.http2.Http2FrameCodec$FrameListener.onDataRead(Http2FrameCodec.java:576)
	at io.netty.handler.codec.http2.Http2FrameListenerDecorator.onDataRead(Http2FrameListenerDecorator.java:36)
	at io.netty.handler.codec.http2.Http2EmptyDataFrameListener.onDataRead(Http2EmptyDataFrameListener.java:49)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onDataRead(DefaultHttp2ConnectionDecoder.java:292)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readDataFrame(DefaultHttp2FrameReader.java:422)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:251)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:174)
	at io.netty.handler.codec.http2.DecoratingHttp2ConnectionDecoder.decodeFrame(DecoratingHttp2ConnectionDecoder.java:63)
	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:505)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
	at io.netty.handler.codec.http2.Http2MultiplexCodec.channelRead(Http2MultiplexCodec.java:267)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:352)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1422)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:931)
	at io.netty.channel.embedded.EmbeddedChannel.writeInbound(EmbeddedChannel.java:343)
	at io.netty.handler.codec.http2.Http2FrameInboundWriter$WriteInboundChannelHandlerContext.writeAndFlush(Http2FrameInboundWriter.java:301)
	at io.netty.handler.codec.http2.Http2FrameInboundWriter$WriteInboundChannelHandlerContext.write(Http2FrameInboundWriter.java:295)
	at io.netty.handler.codec.http2.DefaultHttp2FrameWriter.writeData(DefaultHttp2FrameWriter.java:179)
	at io.netty.handler.codec.http2.Http2FrameInboundWriter.writeInboundData(Http2FrameInboundWriter.java:54)
	at io.netty.handler.codec.http2.Http2MultiplexTest.useReadWithoutAutoReadBuffered(Http2MultiplexTest.java:1149)
	at io.netty.handler.codec.http2.Http2MultiplexTest.useReadWithoutAutoReadInRead(Http2MultiplexTest.java:1098)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)

Does this sound right ?

Will dig into it :)

@ti2ger92
Copy link
Author

That looks about right to me, yes. I appreciate that.

@normanmaurer
Copy link
Member

@ti2ger92 can you check #9664 again ? I think it should fix your problem.

@rschmitt
Copy link

@normanmaurer Speaking for @ti2ger92, commit d42aa46 from the window_update_failure branch appears to fix our integration tests.

@normanmaurer
Copy link
Member

@rschmitt can you re-test with the current code of the #9664 PR again ?

@rschmitt
Copy link

@normanmaurer I've run the tests many times with the current code, everything is working great.

@ti2ger92
Copy link
Author

As I commented on the CR its self, my tests also show this solves the problem.

normanmaurer added a commit that referenced this issue Oct 22, 2019
#9664)


Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663
@normanmaurer
Copy link
Member

@rschmitt @ti2ger92 thanks a lot... I just merged the change

normanmaurer added a commit that referenced this issue Oct 22, 2019
#9664)


Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663
@normanmaurer normanmaurer added this to the 4.1.43.Final milestone Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants