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

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close #9094

Merged

Conversation

Projects
None yet
3 participants
@Scottmitch
Copy link
Member

commented Apr 26, 2019

Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.

Modifications:

  • Http2ConnectionHandler supports an additional boolean constructor argument to
    opt out of close(..) going through the graceful close path.
  • Http2FrameCodecBuilder and Http2MultiplexCodec expose
    gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
    are already exposed we should hook them up and make sure the timeout is applied
    properly.
  • Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
    initiate the graceful closure process after writing a GOAWAY frame if the error
    code is NO_ERROR. This means that writing a Http2GoAwayFrame from
    Http2FrameCodec will initiate graceful close.

Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.

@Scottmitch Scottmitch requested review from normanmaurer and ejona86 Apr 26, 2019

@Scottmitch Scottmitch force-pushed the Scottmitch:http2_multiplex_decouple_goaway_and_close branch from a527eac to 57f8cc5 Apr 26, 2019

@@ -814,6 +837,12 @@ public void operationComplete(ChannelFuture future) throws Exception {
}
});
}
// if closeListener != null this means we have already initiated graceful closure. doGracefulShutdown will apply
// the gracefulShutdownTimeoutMillis on each invocation, and it is possible the timeout value was changed

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 26, 2019

Author Member

@ejona86 - there is some complexity related to preserving existing functionality and gracefulShutdownTimeoutMillis management (which we should re-asses for 5.0).

@ejona86
Copy link
Member

left a comment

Couple of comments for things that may get looked at, but overall LGTM

// new ClosingChannelFutureListener when the graceful close completes if the promise is not null.
if (closeListener == null) {
closeListener = tmp;
} else if (promise != null) {

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 26, 2019

Member

To confirm I understand: this is a bug fix that was just noticed when messing with this PR. It isn't necessary for the new feature (any more than previously).

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 26, 2019

Author Member

A null promise was possible but wasn't likely (e.g. would have to come from the pipeline, or from the ChanntHandlerContext promise factory). However now we are explicitly passing null when we don't care about being notified when the operation completes (e.g. after we write a GOAWAY we don't really care about knowing when graceful close completes).

Show resolved Hide resolved ...2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java Outdated
}

private void doClose() {
if (promise == null) {

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 26, 2019

Member

Why can promise now be null? I don't see why this is done.

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 26, 2019

Member

Oh, I see it now, on line 844.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 26, 2019

Author Member

yip this class (e.g. ClosingChannelFutureListener) is a bit overloaded as it manages 3 things: promise propagation (optional), timer management (optional), and connection closure. I debated breaking it up ... but decided to just leave it as is for now due to its internal nature and easy to change if necessary.

@ejona86

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Oh! A present for me!

Minor note: I'm not 100% confident we should have the gracefulShutdownTimeoutMillis convenience in the new http2 API, or what it should look like.

The behavior of gracefulShutdownTimeoutMillis is sort of easy: you set a timer and call channel.close() when it expires.

Although to do the goaway itself really well you would send a PING after the GOAWAY and when the PING ACK arrives send out the second GOAWAY. Although that needs a configuration value for when to timeout the ping and send a second GOAWAY anyway. We have this implemented in gRPC and use a hard-coded timeout of 10 seconds. The double-goaway is described in the RFC, but using PING was not.

Of the two, the two-stage GOAWAY seems much more useful to implement in-library. Anyway, I'm rambling and it is just stuff to think about.

@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

Minor note: I'm not 100% confident we should have the gracefulShutdownTimeoutMillis convenience in the new http2 API, or what it should look like.

I made a similar comment #9094 (comment) around the same time 👍. I agree it is better to move higher in the stack. The rational for filling out the implementation here is because it is already exposed in the public APIs for the frame/multiplex builders and just not hooked up. We could consider this as "broken" and mark it deprecated but I'm not sure if it does much damage to implement it as the APIs describe. Folks can always explicitly disable it (e.g. set to -1) at the higher level, we can then move/improve/remove for 5.0.

I also agree the GOAWAY(<max id>), PING, PING(ACK), GOAWAY(<real id>) approach should work well, but also a bit more involved. IIRC we also don't automate the GOAWAY(<max id>), GOAWAY(<read id>) in Netty. We leave the "which stream ID goes in the go away" and "how many goaways are sent" as exercises to the user. I'm fine with leaving this as a "higher layer" responsibility for now and reassessing for 5.0 (as long as Netty doesn't prevent it from happening).

wdyt?

@ejona86

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

To be clear, my "minor note" was more about future direction vs holding up this PR.

I'm not too worried about gracefulShutdownTimeoutMillis. It is easy to implement wherever we need it. If it isn't implemented in the builder, that may be a reason to remove it vs add it, but meh. I'm not really worried.

We leave the "which stream ID goes in the go away" and "how many goaways are sent" as exercises to the user. I'm fine with leaving this as a "higher layer" responsibility for now and reassessing for 5.0

Yeah. I wouldn't necessarily say "5.0", but "later" SGTM and I'm fine if it only makes it into 5.0. It might just be a utility function.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@Scottmitch please fix the leak :)

@normanmaurer
Copy link
Member

left a comment

LGTM after leak is fixed

Scottmitch added some commits Apr 25, 2019

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY grac…
…eful close

Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.

Modifications:
- Http2ConnectionHandler supports an additional boolean constructor argument to
opt out of close(..) going through the graceful close path.
- Http2FrameCodecBuilder and Http2MultiplexCodec expose
 gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
are already exposed we should hook them up and make sure the timeout is applied
properly.
- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
initiate the graceful closure process after writing a GOAWAY frame if the error
code is NO_ERROR. This means that writing a Http2GoAwayFrame from
Http2FrameCodec will initiate graceful close.

Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.

@Scottmitch Scottmitch force-pushed the Scottmitch:http2_multiplex_decouple_goaway_and_close branch from 022bb86 to 2ffd428 Apr 27, 2019

@normanmaurer normanmaurer added this to the 4.1.36.Final milestone Apr 28, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@Scottmitch feel free to merge once satisfied (also please cherry-pick into master)

@Scottmitch Scottmitch merged commit b4e3c12 into netty:4.1 Apr 29, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details

@Scottmitch Scottmitch deleted the Scottmitch:http2_multiplex_decouple_goaway_and_close branch Apr 29, 2019

Scottmitch added a commit that referenced this pull request Apr 29, 2019

Http2ConnectionHandler to allow decoupling close(..) from GOAWAY grac…
…eful close (#9094)

Motivation:
Http2ConnectionHandler#close(..) always runs the GOAWAY and graceful close
logic. This coupling means that a user would have to override
Http2ConnectionHandler#close(..) to modify the behavior, and the
Http2FrameCodec and Http2MultiplexCodec are not extendable so you cannot
override at this layer. Ideally we can totally decouple the close(..) of the
transport and the GOAWAY graceful closure process completely, but to preserve
backwards compatibility we can add an opt-out option to decouple where the
application is responsible for sending a GOAWAY with error code equal to
NO_ERROR as described in https://tools.ietf.org/html/rfc7540#section-6.8 in
order to initiate graceful close.

Modifications:
- Http2ConnectionHandler supports an additional boolean constructor argument to
opt out of close(..) going through the graceful close path.
- Http2FrameCodecBuilder and Http2MultiplexCodec expose
 gracefulShutdownTimeoutMillis but do not hook them up properly. Since these
are already exposed we should hook them up and make sure the timeout is applied
properly.
- Http2ConnectionHandler's goAway(..) method from Http2LifecycleManager should
initiate the graceful closure process after writing a GOAWAY frame if the error
code is NO_ERROR. This means that writing a Http2GoAwayFrame from
Http2FrameCodec will initiate graceful close.

Result:
Http2ConnectionHandler#close(..) can now be decoupled from the graceful close
process, and immediately close the underlying transport if desired.
@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

master (67518e3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.