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

HTTP/2 to support asynchronous SETTINGS ACK #9069

Merged
merged 8 commits into from Apr 25, 2019

Conversation

Projects
None yet
4 participants
@Scottmitch
Copy link
Member

commented Apr 17, 2019

Motivation:
The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS
ACK before the application sees the SETTINGS frame. The application may need to
adjust its state depending upon what is in the SETTINGS frame before applying
the remote settings and responding with an ACK (e.g. to adjust for max
concurrent streams). In order to accomplish this the HTTP/2 codec should allow
for the application to opt-in to sending the SETTINGS ACK.

Modifications:

  • DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can
    be queued instead of immediately applying and ACKing.
  • DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it
    exists) to apply the earliest received but not yet ACKed SETTINGS frame.
  • AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new
    option to enable the application to opt-in to managing SETTINGS ACK.

Result:
HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.

HTTP/2 to support asynchronous SETTINGS ACK
Motivation:
The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS
ACK before the application sees the SETTINGS frame. The application may need to
adjust its state depending upon what is in the SETTINGS frame before applying
the remote settings and responding with an ACK (e.g. to adjust for max
concurrent streams). In order to accomplish this the HTTP/2 codec should allow
for the application to opt-in to sending the SETTINGS ACK.

Modifications:
- DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can
  be queued instead of immediately applying and ACKing.
- DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it
  exists) to apply the earliest received but not yet ACKed SETTINGS frame.
- AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new
  option to enable the application to opt-in to managing SETTINGS ACK.

Result:
HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.

@Scottmitch Scottmitch requested a review from normanmaurer Apr 17, 2019

Scottmitch added some commits Apr 17, 2019

@normanmaurer normanmaurer requested a review from ejona86 Apr 17, 2019

@normanmaurer

This comment has been minimized.

@Scottmitch Scottmitch requested a review from normanmaurer Apr 17, 2019

@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) {
if (msg instanceof Http2SettingsAckFrame) {
serverAckLatch.countDown();

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 18, 2019

Member

nit: maybe also verify we do not receive more of these frame as expected.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 18, 2019

Author Member

I changed the test to more carefully verify the first ACK and the second ACK, but I'll just leave it at that for now. Verifying more may require a timeout and this would be somewhat unreliable.

@normanmaurer
Copy link
Member

left a comment

LGTM... 2 nits

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

still LGTM

@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

build failure attributed to #9075

@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

test this please

* h2 semantics on top of the frames.
* @param requestVerifier Determines if push promised streams are valid.
* @param autoAckSettings {@code false} to disable automatically applying and sending settings acknowledge frame.
* In this case see see {@link #pollReceivedSettings()} which is expected to be consumed by the

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 24, 2019

Member

Please indent at least 2 spaces.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 25, 2019

Author Member

done

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 25, 2019

Member

You missed the next two lines.


public DefaultHttp2ConnectionEncoder(Http2Connection connection, Http2FrameWriter frameWriter) {
this.connection = checkNotNull(connection, "connection");
this.frameWriter = checkNotNull(frameWriter, "frameWriter");
remoteSettingsSupplier = EmptyHttp2SettingsReceivedSupplier.INSTANCE;

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 24, 2019

Member

This scares me. It is really subtle. If a user sets autoAckSettingsFrame(false), then they must also set this, otherwise settings management will silently break and be horrible to debug. I see AbstractHttp2ConnectionHandlerBuilder configures this, but that's not the only way these can be connected.

There's quite a bit of plumbing here in general. It seems it would be better to have encoder.writeSettingsAck() be passed the Settings that is being Acked. That would mean we'd also remove remoteSettings(). Yes, that breaks API, but both methods can only be sanely called from the Decoder today and I doubt many people have implemented their own Decoder.

It would mean that Http2FrameCodec would need the Settings queue, but it would be an internal detail and not cross API boundaries.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 25, 2019

Author Member

if a user sets autoAckSettingsFrame(false), then they must also set this..

A user that provides their own encoder/decoer on the builder will not be able to use autoAckSettingsFrame on the builder (the builder will throw). The builder exposes autoAckSettingsFrame and this is how it is implemented under the hood. The remoteSettingsSupplier is an implementation detail from the perspective of autoAckSettingsFrame on the builder, and shouldn't lead to an inconsistent state.

It seems it would be better to have encoder.writeSettingsAck() be passed the Settings that is being Acked.

I don't think passing the settings to ACK into encoder.writeSettingsAck() is a good thing because it may lead to folks trying to ACK settings out of order, which would result in protocol violations. This would be more likely now that async settings ACK is permitted. The HTTP/2 protocol doesn't allow for ACKing specific settings frames. Each ACK applies to the earliest sent SETTINGs frame that has not yet been ACKed.

That would mean we'd also remove remoteSettings(). Yes, that breaks API.

I agree the existing dependencies between encoder/decoder and associated implementation is not ideal. However I don't want to change the existing design (which already has this dependency for applying local settings sent from the encoder, in the decoder when an ACK is received, see Http2ConnectionEncoder) with this PR. I don't think this warrants breaking the APIs for 4.1 as the approach is consistent (protocol logic lives in encoder/decoder), but we should revisit for 5.0 when we move everything to the frame/multiplex codec.

This comment has been minimized.

Copy link
@ejona86

ejona86 Apr 25, 2019

Member

A user that provides their own encoder/decoer on the builder

Not every user uses the builder. It is not required. I agree that if you use the builder things look fine.

I don't think passing the settings to ACK into encoder.writeSettingsAck() is a good thing because it may lead to folks trying to ACK settings out of order

Shoot. Yeah. Then maybe can the queue be moved to the encoder? The decoder already references the encoder, so that seems much easier. I'm imagining code like:

public void onSettingsRead(final ChannelHandlerContext ctx, Http2Settings settings) throws Http2Exception {
    encoder.remoteSettingsReceived(settings);
    if (autoAckSettings) {
        encoder.writeSettingsAck(ctx, ctx.newPromise());
    }
}

This doesn't seem like an invasive re-design, any more than this PR is already doing.

This comment has been minimized.

Copy link
@Scottmitch

Scottmitch Apr 25, 2019

Author Member

Then maybe can the queue be moved to the encoder? The decoder already references the encoder, so that seems much easier

IIUC we are no longer breaking APIs/behavior for 4.1 so we cannot modify Http2ConnectionEncoder directly. Http2ConnectionEncoder has a remoteSettings method but queueing in the implementation of this method would break the semantics of this method. I will make some modifications to preserve the existing API/semantics and put the queue in the encoder. PTAL

Show resolved Hide resolved ...c/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java Outdated

Scottmitch added some commits Apr 25, 2019

@Scottmitch Scottmitch requested a review from ejona86 Apr 25, 2019

@ejona86
Copy link
Member

left a comment

outstandingRemoteSettingsQueue puts the encoder in a different "mode", but any pain seems to be to maintain API compatibility and is relatively small and contained. Thank you!

@Scottmitch

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

thanks for review @ejona86! agreed we should do this differently in 5.0 but lets accept the pain for 4.1.

@Scottmitch Scottmitch merged commit b3dba31 into netty:4.1 Apr 25, 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:async_settings_ack branch Apr 25, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@Scottmitch can you open an issue for 5 so we do not forget ?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

I will just cherry-pick it into master as well for now to keep the code-base as similar as possible and we can revisit when we tackle http2 for Netty 5.

normanmaurer added a commit that referenced this pull request Apr 28, 2019

HTTP/2 to support asynchronous SETTINGS ACK (#9069)
Motivation:
The HTTP/2 codec will synchronously respond to a SETTINGS frame with a SETTINGS
ACK before the application sees the SETTINGS frame. The application may need to
adjust its state depending upon what is in the SETTINGS frame before applying
the remote settings and responding with an ACK (e.g. to adjust for max
concurrent streams). In order to accomplish this the HTTP/2 codec should allow
for the application to opt-in to sending the SETTINGS ACK.

Modifications:
- DefaultHttp2ConnectionDecoder should support a mode where SETTINGS frames can
  be queued instead of immediately applying and ACKing.
- DefaultHttp2ConnectionEncoder should attempt to poll from the queue (if it
  exists) to apply the earliest received but not yet ACKed SETTINGS frame.
- AbstractHttp2ConnectionHandlerBuilder (and sub classes) should support a new
  option to enable the application to opt-in to managing SETTINGS ACK.

Result:
HTTP/2 allows for asynchronous SETTINGS ACK managed by the application.
@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Ok done ... also cherry-picked ec62af0

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.