Commit 306299323cd seems to have broken finagle tests #9395
Labels
Milestone
Comments
|
That’s interesting... keep me posted |
|
@bryce-anderson btw as this was only some cleanup we can also revert the commit while trying to figure out what's wrong. WDYT ? |
|
Let me try master with this reverted and we can go from there. |
|
thanks ... |
|
Revert does fix it. It's a small enough diff that we might be able to sort it out pretty quick. Do you want to wait until tomorrow before reverting to give me a few hours to check how to fix it? The revert isn't 100% clean. |
|
Yeah sounds good |
|
Added some comments to the commit. |
normanmaurer
added a commit
that referenced
this issue
Jul 23, 2019
Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Adjust unit test to capture the bug Result: Fixes #9395
normanmaurer
added a commit
that referenced
this issue
Jul 23, 2019
… and the correct handler is used Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Beside this we also did not use the correct handler for the upgrade stream in some cases Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Consolidate logic of creating stream channels - Adjust unit test to capture the bug Result: Fixes #9395
normanmaurer
added a commit
that referenced
this issue
Jul 23, 2019
… and the correct handler is used (#9396) Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Beside this we also did not use the correct handler for the upgrade stream in some cases Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Consolidate logic of creating stream channels - Adjust unit test to capture the bug Result: Fixes #9395
normanmaurer
added a commit
that referenced
this issue
Jul 23, 2019
… and the correct handler is used (#9396) Motivation: 3062993 introduced some code change to move the responsibility of creating the stream for the upgrade to Http2FrameCodec. Unfortunaly this lead to the situation of having newStream().setStreamAndProperty(...) be called twice. Because of this we only ever saw the channelActive(...) on Http2StreamChannel but no other events as the mapping was replaced on the second newStream().setStreamAndProperty(...) call. Beside this we also did not use the correct handler for the upgrade stream in some cases Modifications: - Just remove the Http2FrameCodec.onHttpClientUpgrade() method and so let the base class handle all of it. The stream is created correctly as part of the ConnectionListener implementation of Http2FrameCodec already. - Consolidate logic of creating stream channels - Adjust unit test to capture the bug Result: Fixes #9395
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Expected behavior
Using the 4.1.38.Final-SNAPSHOT would not break out internal tests
Actual behavior
Tests broken
Steps to reproduce
Checkout commit 3062993 (or later) and run the finagle tests.
Netty version
Snapshot SHA 3062993.
I don't yet know why it's broken, it could be finagle depending on a bug, but I wanted to file a ticket to let you know so if we need a fix we can get it into 4.1.38.Final. I'm looking into the issue now.
The text was updated successfully, but these errors were encountered: