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.

Sign up for GitHub

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

Jump to bottom

HTTP/2 set initial settings #2341

Merged
merged 4 commits into from
Sep 8, 2022
Merged

HTTP/2 set initial settings #2341

merged 4 commits into from
Sep 8, 2022

Conversation

Scottmitch
Copy link
Member

Motivaiton:
There is no way to define initial values for
http/2 settings.

Modifications:

  • Enhance H2ProtocolConfigBuilder to accept Map<Character, Integer> to
    set the initial settings values.
  • Set the default value of settings with bound max header list size
    and 1mb default initial flow control window.

@Scottmitch Scottmitch marked this pull request as ready for review September 3, 2022 20:48
@Scottmitch Scottmitch force-pushed the h2_settings branch 2 times, most recently from 7b0e71c to 266ca6b Compare September 3, 2022 20:58
Scottmitch
Scottmitch commented Sep 3, 2022
View reviewed changes
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpProtocolConfigs.java
@@ -52,6 +54,11 @@ public static H1ProtocolConfigBuilder h1() {
/**
* Returns {@link H2ProtocolConfig} with the default configuration for
* <a href="https://tools.ietf.org/html/rfc7540">HTTP/2</a>.
* <p>
* Note this doesn't necessarily provide {@link H2ProtocolConfig#initialSettings()} that corresponds to
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth calling out for visibility... default values were set based upon experience and this carve out is meant to highlight that folks should be explicit if they depend upon specific values (defaults are subject to change).

@Scottmitch Scottmitch force-pushed the h2_settings branch 2 times, most recently from bc57b6d to ac3ba07 Compare September 3, 2022 21:47
@Scottmitch
Copy link
Member Author

test failure attributed to #2343

@Scottmitch
HTTP/2 set initial settings
c324d8a 
Motivaiton:
There is no way to define initial values for
[http/2 settings](https://datatracker.ietf.org/doc/html/rfc7540#section-6.5.2).

Modifications:
- Enhance H2ProtocolConfigBuilder to accept Map<Character, Integer> to
  set the initial settings values.
- Set the default value of settings with bound max header list size
  and 1mb default initial flow control window.
- Increase flow control quantum to 16kb because the flow control window
  is larger. This allows streams to write an entire frame per write
  operation and increases goodput on the connection.
idelpivnitskiy
idelpivnitskiy requested changes Sep 7, 2022
View reviewed changes
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2Settings.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2Settings.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2SettingsBuilder.java Show resolved Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java Outdated Show resolved Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ServerParentChannelInitializer.java Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfig.java Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfig.java Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfigBuilder.java Outdated Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfigBuilder.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2SettingsBuilder.java Show resolved Hide resolved
idelpivnitskiy
idelpivnitskiy reviewed Sep 8, 2022
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, last findings:

servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2Settings.java Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2SettingsBuilder.java Outdated Show resolved Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java Outdated Show resolved Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java Show resolved Hide resolved
...alk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java Outdated Show resolved Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfig.java
@@ -60,6 +61,35 @@ default String alpnId() {
@Nullable
KeepAlivePolicy keepAlivePolicy();

/**
* Get the {@link Http2Settings} that provides a hint for the initial settings. Note that some settings may be
* ignored if not supported (e.g. push promise).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider clarifying that we throw an exception instead of ignoring

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the behavior may vary depending upon the use site and implementation, some settings maybe OK to ignore ... I don't think we need to over specify exception usage (illegal argument exception may always be thrown).

servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ProtocolConfig.java Outdated Show resolved Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/Http2SettingsBuilder.java Outdated Show resolved Hide resolved
@Scottmitch
Copy link
Member Author

build failure caused by #2345

@Scottmitch Scottmitch merged commit 40d2f02 into apple:main Sep 8, 2022
@Scottmitch Scottmitch deleted the h2_settings branch September 8, 2022 02:54
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 8, 2022
@idelpivnitskiy
Revert default HTTP/2 settings sent by the client
6181bb5 
Motivation:

apple#2341 changed default HTTP/2 settings that client sends to notify the
server it does not support Server Push.

Modification:

- Set `ENABLE_PUSH=0, MAX_CONCURRENT_STREAMS=0` in settings frame that
client sends to a server;
- Validate custom initial settings that users pass to `H2ProtocolConfig`
and throw if users try to override ENABLE_PUSH/MAX_CONCURRENT_STREAMS on
the client or set ENABLE_PUSH other than 0 for the server;
- Include content message in GO_AWAY frame if client receives a Push
Stream frame;

Result:

Default client behavior doesn't change, custom settings validated to
prevent programming mistakes.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Sep 8, 2022
Merged
idelpivnitskiy added a commit that referenced this pull request Sep 9, 2022
@idelpivnitskiy
Revert default HTTP/2 settings sent by the client (#2346)
bb54241 
Motivation:

#2341 changed default HTTP/2 settings that client sends to notify the
server it does not support Server Push.

Modification:

- Set `ENABLE_PUSH=0, MAX_CONCURRENT_STREAMS=0` in settings frame that
client sends to a server;
- Validate custom initial settings that users pass to `H2ProtocolConfig`
and throw if users try to override ENABLE_PUSH/MAX_CONCURRENT_STREAMS on
the client or set ENABLE_PUSH other than 0 for the server;
- Include content message in GO_AWAY frame if client receives a Push
Stream frame;

Result:

Default client behavior doesn't change, custom settings validated to
prevent programming mistakes.
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 12, 2022
@idelpivnitskiy
Make H2ProtocolConfig interface backward compatible
2044b00 
Motivation:

apple#2341 added 3 new methods on `H2ProtocolConfig` interface but didn't
provide default implementations.

Modifications:

- Add default impls for `initialSettings()`, `flowControlQuantum()`,
and `flowControlWindowIncrement()`;

Result:

`H2ProtocolConfig` is backward compatible with 0.42.16 release.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Sep 12, 2022
Merged
idelpivnitskiy added a commit to idelpivnitskiy/servicetalk that referenced this pull request Sep 12, 2022
@idelpivnitskiy
Make H2ProtocolConfig interface backward compatible
f9d399e 
Motivation:

apple#2341 added 3 new methods on `H2ProtocolConfig` interface but didn't
provide default implementations.

Modifications:

- Add default impls for `initialSettings()`, `flowControlQuantum()`,
and `flowControlWindowIncrement()`;

Result:

`H2ProtocolConfig` is backward compatible with 0.42.16 release.
idelpivnitskiy added a commit that referenced this pull request Sep 12, 2022
@idelpivnitskiy
Make H2ProtocolConfig interface backward compatible (#2361)
93ff14e 
Motivation:

#2341 added 3 new methods on `H2ProtocolConfig` interface but didn't
provide default implementations.

Modifications:

- Add default impls for `initialSettings()`, `flowControlQuantum()`,
and `flowControlWindowIncrement()`;

Result:

`H2ProtocolConfig` is backward compatible with 0.42.16 release.
@idelpivnitskiy idelpivnitskiy mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelpivnitskiy idelpivnitskiy idelpivnitskiy requested changes

@tkountis tkountis Awaiting requested review from tkountis

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Scottmitch @idelpivnitskiy