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

Match Http2ClientUpgradeCodec to the new upgrade policy #7227

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@mosesn
Contributor

mosesn commented Sep 19, 2017

Motivation:

We changed Http2ConnectionHandler to expect the upgrade method to be
called after we send the preface (ie add the handler to the pipeline)
but we forgot to change the Http2ClientUpgradeCodec to match the new
policy. This meant that client-side h2c upgrades failed.

Modifications:

Reverse sending the preface and calling the upgrade method to match the
new policy.

Result:

Fixes #7226. Clients can initiate h2c upgrades successfully.

@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Sep 19, 2017

Contributor

I haven't written tests yet because I'm running out the door, but I'll write them tomorrow.

Contributor

mosesn commented Sep 19, 2017

I haven't written tests yet because I'm running out the door, but I'll write them tomorrow.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 19, 2017

Member

LGTM... please ping me once a test is added and I will merge. @mosesn thanks!

Member

normanmaurer commented Sep 19, 2017

LGTM... please ping me once a test is added and I will merge. @mosesn thanks!

Match Http2ClientUpgradeCodec to the new upgrade policy
Motivation:

We changed Http2ConnectionHandler to expect the upgrade method to be
called *after* we send the preface (ie add the handler to the pipeline)
but we forgot to change the Http2ClientUpgradeCodec to match the new
policy.  This meant that client-side h2c upgrades failed.

Modifications:

Reverse sending the preface and calling the upgrade method to match the
new policy.

Result:

Clients can initiate h2c upgrades successfully.
@mosesn

This comment has been minimized.

Show comment
Hide comment
@mosesn

mosesn Sep 19, 2017

Contributor
Contributor

mosesn commented Sep 19, 2017

channel.flush();
codec.upgradeTo(ctx, null);
assertTrue(channel.pipeline().get("connectionHandler") != null);

This comment has been minimized.

@normanmaurer

normanmaurer Sep 20, 2017

Member

@mosesn do we also need to call channel.finish() and assert the return value here ? Also what about releasing any written ByteBuf ?

@normanmaurer

normanmaurer Sep 20, 2017

Member

@mosesn do we also need to call channel.finish() and assert the return value here ? Also what about releasing any written ByteBuf ?

This comment has been minimized.

@normanmaurer

normanmaurer Sep 20, 2017

Member

@mosesn ping :) Would love to pull this in before the release

@normanmaurer

normanmaurer Sep 20, 2017

Member

@mosesn ping :) Would love to pull this in before the release

@Scottmitch

lgtm after @normanmaurer comment addressed

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 20, 2017

Member

Cherry-picked into 4.1 (1ff2e1f) after addressing my comments ...

Thanks @mosesn

Member

normanmaurer commented Sep 20, 2017

Cherry-picked into 4.1 (1ff2e1f) after addressing my comments ...

Thanks @mosesn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment