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

Trigger user event when H2 conn preface & SETTINGS frame is sent #7330

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@lionelli1

lionelli1 commented Oct 23, 2017

Motivation:
Previously client Http2ConnectionHandler trigger a user event
immediately when the HTTP/2 connection preface is sent. Any attempt to
immediately send a new request would cause the server to terminate the
connection, as it might not have received the SETTINGS frame from the
client. Per RFC7540 Section 3.5, the preface "MUST be followed by a
SETTINGS frame (Section 6.5), which MAY be empty."
(https://tools.ietf.org/html/rfc7540#section-3.5)

This event could be made more meaningful if it also indicates that the
initial client SETTINGS frame has been sent to signal that the channel
is ready to send new requests.

Modification:

  • Renamed event to Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.
  • Modified Http2ConnectionHandler to trigger the user event only if it
    is a client and it has sent both the preface and SETTINGS frame.

Result:
It is now safe to use the event as an indicator that the HTTP/2
connection is ready to send new requests.

@lionelli1

This comment has been minimized.

Show comment
Hide comment
@lionelli1

lionelli1 commented Oct 23, 2017

@normanmaurer @Scottmitch PTAL thanks!

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 23, 2017

Member

@ejona86 @carl-mastrangelo PTAL... imho this is the correct thing to do but would like to have your review as well as it may also affect GRPC.

Member

normanmaurer commented Oct 23, 2017

@ejona86 @carl-mastrangelo PTAL... imho this is the correct thing to do but would like to have your review as well as it may also affect GRPC.

@Scottmitch Scottmitch self-assigned this Oct 23, 2017

@Scottmitch Scottmitch added the feature label Oct 23, 2017

@Scottmitch Scottmitch added this to the 4.1.17.Final milestone Oct 23, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Oct 23, 2017

thanks @lionelli1 !

@@ -231,7 +231,7 @@ void handlerAdded0(@SuppressWarnings("unsed") ChannelHandlerContext ctx) throws
*/
@Override
public final void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
if (evt instanceof Http2ConnectionPrefaceWrittenEvent) {
if (evt == Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.INSTANCE) {

This comment has been minimized.

@Scottmitch

Scottmitch Oct 23, 2017

Member

lets be consistent and either use evt == Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.INSTANCE or instanceof ... instanceof allows us to be less dependent on the class hierarchy but I think either way will work.

@Scottmitch

Scottmitch Oct 23, 2017

Member

lets be consistent and either use evt == Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.INSTANCE or instanceof ... instanceof allows us to be less dependent on the class hierarchy but I think either way will work.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 24, 2017

Member

@lionelli1 @Scottmitch I vote for using == here. @lionelli1 please make the change and squash. After this is done i will merge it.

@normanmaurer

normanmaurer Oct 24, 2017

Member

@lionelli1 @Scottmitch I vote for using == here. @lionelli1 please make the change and squash. After this is done i will merge it.

This comment has been minimized.

@lionelli1

lionelli1 Oct 24, 2017

ok! replaced all instanceof with == and squashed my changes. thanks!

@lionelli1

lionelli1 Oct 24, 2017

ok! replaced all instanceof with == and squashed my changes. thanks!

Trigger user event when H2 conn preface & SETTINGS frame are sent
Motivation:
Previously client Http2ConnectionHandler trigger a user event
immediately when the HTTP/2 connection preface is sent. Any attempt to
immediately send a new request could cause the server to terminate the
connection, as it might not have received the SETTINGS frame from the
client. Per RFC7540 Section 3.5, the preface "MUST be followed by a
SETTINGS frame (Section 6.5), which MAY be empty."
(https://tools.ietf.org/html/rfc7540#section-3.5)

This event could be made more meaningful if it also indicates that the
initial client SETTINGS frame has been sent to signal that the channel
is ready to send new requests.

Modification:
- Renamed event to Http2ConnectionPrefaceAndSettingsFrameWrittenEvent.
- Modified Http2ConnectionHandler to trigger the user event only if it
  is a client and it has sent both the preface and SETTINGS frame.

Result:
It is now safe to use the event as an indicator that the HTTP/2
connection is ready to send new requests.

@normanmaurer normanmaurer self-assigned this Oct 24, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

Cherry-picked into 4.1 (baf273a).

@lionelli1 thanks a lot!

Member

normanmaurer commented Oct 24, 2017

Cherry-picked into 4.1 (baf273a).

@lionelli1 thanks a lot!

@lionelli1 lionelli1 deleted the lionelli1:prefaceAndSettingSent branch Oct 24, 2017

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