Make HTTP/2 flow control window size configurable for clients and servers #786

johnkassebaum · 2020-05-08T23:47:43Z

Make HTTP/2 flow control target window size configurable in a client or server HTTP2 connection.
Motivation:

Higher streaming thruput between server and client is available by the either specifying a HTTP/2 flow control window size higher than the initial value of 2^16-1. The target value needs to be passed either using SETTINGS_INITIAL_WINDOW_SIZE in the connection preface, or via a WINDOW_UPDATE. As implemented here, the HTTP2FlowControlWindow passes the value configured in a windowUpdate.

Modifications:

For the client:
Added property targetWindowSize to ClientConnection.Configuration.
In the existing ClientConnection.Configuration.init, sets the new targetWindowSize property to the initial flow-control window size (65535).
Duplicated ClientConnection.Configuration.init and added a targetWindowSize arg.
Added property and setter for targetWindowSize to ClientConnection.Builder.
During client bootstrap, pass configuration.targetWindowSize to the channel initializer.
Duplicated channel initializer configureGRPCClient and added a targetWindowSize arg.
Duplicated all tests that directly invoke the original ClientConnection.Configuration.init to explicitly pass targetWindowSizeto the new initializer.

For the server:
Added property httpTargetWindowSize to Server.Configuration.
In Server.Configuration.init, added httpTargetWindowSize arg with default value 2^16-1.
Added property and setter for httpTargetWindowSize to Server.Builder.
During server bootstrap in Server.start, pass configuration.httpTargetWindowSize to the HTTPProtocolSwitcher.
Configure http/2 flow control target window size on the http2 pipeline in HTTPProxySwitcher.
Result:

One can build a ClientConnection or Serverwith a target window size value that is configured via a WINDOW_UPDATE after the connection is established.

linux-foundation-easycla · 2020-05-08T23:47:46Z

The committers are authorized under a signed CLA.

✅ John Kassebaum (2a1fa74)

Lukasa

Broadly I think this looks really good! We should aim to get a HTTP/2 release out early this week that can unblock this PR.

Sources/GRPC/ClientConnection.swift

glbrntt · 2020-05-11T08:53:27Z

Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift

@@ -31,6 +31,7 @@ extension ClientConnection {
 extension ClientConnection {
  public class Builder {
    private let group: EventLoopGroup
+    private var targetWindowSize: Int = 65535


@Lukasa should we add a default-target-window-size constant to nio-http2? This is floating around in a number of places.

Alternatively we could just switch the value in the gRPC API to be an optional/enum with "defer to http2" and "use this value" cases.

Sources/GRPC/ClientConnection.swift

glbrntt · 2020-05-11T09:16:01Z

If you confirm that the correct place is where HTTPProtocolSwitcher.channelRead(context:data:), in the case of .http2, invokes Channel.configureHTTP2Pipeline(mode:) then I would add the same targetWindowSize to Server.Configuration and to Server.Builder, and would then pass it to the HTTPProtocolSwitcher in Server.makeBootstrap(configuration:)

That's the right place 👍

glbrntt · 2020-05-29T08:48:50Z

@johnkassebaum FYI your NIO HTTP/2 change was tagged a little while ago

johnkassebaum · 2020-05-29T13:01:44Z

Thanks @glbrntt I saw that. So sorry for the delay on this, was able to get back to it last night and will address comments and add server configuration today.

glbrntt

No problem about the delay @johnkassebaum! This basically looks good, I just have a couple of comments around docs/naming.

glbrntt · 2020-06-01T08:38:24Z

Sources/GRPC/ClientConnection.swift

@@ -324,6 +330,7 @@ extension Channel {
  }

  func configureGRPCClient(
+    httpTargetWindowSize: Int = 65535,


I don't think we need a default here: we should have always have a value from the configuration.

glbrntt · 2020-06-01T08:41:14Z

Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift

@@ -195,6 +197,15 @@ extension ClientConnection.Builder.Secure {
  }
 }

+extension ClientConnection.Builder {
+  /// Sets the HTTP/2 flow control target window size.


Suggested change

/// Sets the HTTP/2 flow control target window size.

/// Sets the HTTP/2 flow control target window size. Defaults to 65,535 if not explicitly set.

Sources/GRPC/Server.swift

Sources/GRPC/ServerBuilder.swift

glbrntt · 2020-06-01T08:45:34Z

gRPC-Swift.podspec

@@ -18,7 +18,7 @@ Pod::Spec.new do |s|

    s.dependency 'Logging', '1.2.0'
    s.dependency 'SwiftNIO', '2.15.0'
-    s.dependency 'SwiftNIOHTTP2', '1.11.0'
+    s.dependency 'SwiftNIOHTTP2', '1.12.0'


No need to update this; it's a generated file.

glbrntt · 2020-06-01T08:50:20Z

Tests/GRPCTests/ClientTLSFailureTests.swift

@@ -135,6 +149,43 @@ class ClientTLSFailureTests: GRPCTestCase {
      XCTFail("Expected NIOSSLError.handshakeFailed(BoringSSL.sslError)")
    }
  }
+
+  func testClientConnectionFailsWhenServerIsUnknownWithHTTPTargetWindowSize() throws {


Could you remove the extra tests since we're not actually validating a change in behaviour here? I think I'm okay just deferring to NIO HTTP/2 for the tests.

Lukasa

No further notes beyond @glbrntt's.

glbrntt · 2020-06-01T12:39:36Z

Package.swift

@@ -29,7 +29,7 @@ let package = Package(
    // Main SwiftNIO package
    .package(url: "https://github.com/apple/swift-nio.git", from: "2.14.0"),
    // HTTP2 via SwiftNIO
-    .package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.8.0"),
+    .package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.12.0"),


Mind bumping this to 1.12.1? We missed something the first time with the window size: apple/swift-nio-http2#205

…connection. Motivation: Higher stremaing thruput from server to client is available by specifying a flow control window size higher than the initial value of 2^16-1. The target value needs to be passed either using SETTINGS_INITIAL_WINDOW_SIZE in the connection preface, or via a WINDOW_UPDATE. The HTTP2FlowControlWindow passes the value configured here in `windowUpdate`. Modifications: Set package and podspec dependency swift-nio-http2 target to 1.12.0, needed for pipeline configuration with http/2 flow control target window size coniguration. Added property `httpTargetWindowSize` to `ClientConnection.Configuration`. In `ClientConnection.Configuration.init`, added httpTargetWindowSize arg with default value 2^16-1. Added property and setter for `httpTargetWindowSize` to `ClientConnection.Builder`. During client bootstrap in ConnectionManager, pass `configuration.httpTargetWindowSize` to the channel configurator. Configure http/2 flow control target window size on the grpc client pipeline. Duplicated all tests that directly invoke the original `ClientConnection.Configuration.init` to explicitly pass `httpTargetWindowSize` to the new initializer. Result: One can build a `ClientConnection` with a http/2 flow control target window size value that is configured via a WINDOW_UPDATE after the connection is established.

…connection. Motivation: Higher streaming thruput from client to server is available by specifying a flow control window size higher than the initial value of 2^16-1. The target value needs to be passed either using SETTINGS_INITIAL_WINDOW_SIZE in the connection preface, or via a WINDOW_UPDATE. The HTTP2FlowControlWindow passes the value configured here in `windowUpdate`. Modifications: Added property `httpTargetWindowSize` to `Server.Configuration`. In `Server.Configuration.init`, added httpTargetWindowSize arg with default value 2^16-1. Added property and setter for `httpTargetWindowSize` to `Server.Builder`. During server bootstrap in Server.start, pass `configuration.httpTargetWindowSize` to the HTTPProtocolSwitcher. Configure http/2 flow control target window size on the http2 pipeline in HTTPProxySwitcher. Result: One can build a `Server` with a target window size value that is configured via a WINDOW_UPDATE after the connection is established.

Sources/GRPC/ClientConnection.swift

glbrntt

Looks great, thanks @johnkassebaum!

Lukasa approved these changes May 11, 2020

View reviewed changes

glbrntt requested changes May 11, 2020

View reviewed changes

johnkassebaum force-pushed the jk-set-connection-target-window-size branch 3 times, most recently from 8246629 to 65b42eb Compare May 29, 2020 17:24

johnkassebaum requested a review from glbrntt May 29, 2020 17:27

johnkassebaum marked this pull request as ready for review May 29, 2020 17:27

johnkassebaum requested a review from Lukasa May 29, 2020 17:27

johnkassebaum force-pushed the jk-set-connection-target-window-size branch from 65b42eb to 5be66d8 Compare May 29, 2020 19:08

glbrntt requested changes Jun 1, 2020

View reviewed changes

Lukasa approved these changes Jun 1, 2020

View reviewed changes

glbrntt reviewed Jun 1, 2020

View reviewed changes

John Kassebaum added 2 commits June 1, 2020 20:50

johnkassebaum force-pushed the jk-set-connection-target-window-size branch from 5be66d8 to c80bd97 Compare June 2, 2020 03:57

johnkassebaum requested a review from glbrntt June 2, 2020 04:13

glbrntt reviewed Jun 2, 2020

View reviewed changes

Sources/GRPC/ClientConnection.swift Outdated Show resolved Hide resolved

Apply suggestions from code review

9cc9659

glbrntt approved these changes Jun 2, 2020

View reviewed changes

glbrntt merged commit f71049e into grpc:master Jun 2, 2020

glbrntt added the 🆕 semver/minor Adds new public API. label Jun 8, 2020

	/// Sets the HTTP/2 flow control target window size.
	/// Sets the HTTP/2 flow control target window size. Defaults to 65,535 if not explicitly set.

Make HTTP/2 flow control window size configurable for clients and servers #786

Make HTTP/2 flow control window size configurable for clients and servers #786

Uh oh!

Conversation

johnkassebaum commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glbrntt May 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glbrntt commented May 11, 2020

Uh oh!

glbrntt commented May 29, 2020

Uh oh!

johnkassebaum commented May 29, 2020

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

glbrntt Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

glbrntt Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glbrntt Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

glbrntt Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

glbrntt Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glbrntt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnkassebaum commented May 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented May 8, 2020 •

edited

Loading