Http2ConnectionHandler to allow decoupling close(..) from GOAWAY graceful close by Scottmitch · Pull Request #9094 · netty/netty

Scottmitch · 2019-04-26T22:29:46Z

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 · 2019-04-26T23:17:31Z

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

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

ejona86

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

ejona86 · 2019-04-26T23:10:58Z

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

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).

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).

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

ejona86 · 2019-04-26T23:17:57Z

codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java

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

Oh, I see it now, on line 844.

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 · 2019-04-26T23:20:08Z

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 · 2019-04-26T23:32:52Z

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 · 2019-04-26T23:49:40Z

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 · 2019-04-27T05:19:12Z

@Scottmitch please fix the leak :)

normanmaurer

LGTM after leak is fixed

…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.

…compatbiilty

normanmaurer · 2019-04-28T08:50:35Z

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

…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 · 2019-04-29T00:56:07Z

master (67518e3)

Scottmitch requested review from ejona86 and normanmaurer April 26, 2019 22:29

Scottmitch mentioned this pull request Apr 26, 2019

Add Http2GoAwayWriteEvent #9071

Closed

Scottmitch force-pushed the http2_multiplex_decouple_goaway_and_close branch from a527eac to 57f8cc5 Compare April 26, 2019 23:15

Scottmitch commented Apr 26, 2019

View reviewed changes

ejona86 approved these changes Apr 26, 2019

View reviewed changes

normanmaurer approved these changes Apr 27, 2019

View reviewed changes

Scottmitch added 3 commits April 27, 2019 11:05

remove extra flush

01281c6

frame and multiplex coded set gracefulclose timer to 0 for backwards …

2ffd428

…compatbiilty

Scottmitch force-pushed the http2_multiplex_decouple_goaway_and_close branch from 022bb86 to 2ffd428 Compare April 27, 2019 18:08

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

only timeout once on start of graceful close when writing a goaway

ec2ce58

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

Scottmitch deleted the http2_multiplex_decouple_goaway_and_close branch April 29, 2019 00:50

voidzcy mentioned this pull request May 31, 2019

Http2ConnectionHandler#goAway should not initiate channel graceful shutdown #9207

Closed

Uh oh!

Conversation

Scottmitch commented Apr 26, 2019

Uh oh!

Scottmitch Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

ejona86 Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

Scottmitch Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ejona86 Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

ejona86 Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

Scottmitch Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

ejona86 commented Apr 26, 2019

Uh oh!

Scottmitch commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ejona86 commented Apr 26, 2019

Uh oh!

normanmaurer commented Apr 27, 2019

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Apr 28, 2019

Uh oh!

Scottmitch commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Scottmitch commented Apr 26, 2019 •

edited

Loading