Make multiplexer's targetWindowSize configurable #202

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

Merged

Lukasa merged 3 commits into apple:master from johnkassebaum:jk-expose-target-window-size

May 11, 2020

Contributor

johnkassebaum commented May 5, 2020 •

edited

Loading

Make multiplexer's targetWindowSize configurable and use configurability when creating stream channels.

Motivation:
targetWindowSize constrains throughput. The default hardcoded 65535 is an unecessarily low limit that should be otherwise configrable. A TODO in the code stated as such.

Modifications:

In HTTP2StreamMultiplexer, responding to a TODO where stream multiplexer creates a stream channel, created a multiplexr stored property to be used which is set in the default initilaizer.
In HTTP2PipelineHelpers, added versions of configureHTTP2Pipeline(..) and configureCommonHTTP2ServerPipeline(..) that require an activeTargetWindow arg.

Result:
Clients, such as GRPCClient, can configure pipelines with targetWindowSizes to their liking.

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

7 similar comments

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

swift-server-bot commented May 5, 2020

Can one of the admins verify this patch?

Lukasa requested a review from glbrntt

May 5, 2020 13:44

Lukasa requested changes

View reviewed changes

Contributor

Lukasa left a comment

Thanks, this broadly looks really good! I've left some notes in the diff that we need to address.

Additionally, while using Int32 internally is just fine, the public API should use Int. In general, wherever possible in public API it is better to use Int than one of the sized integer types. This allows users to pass numbers around using the currency type, and for them to be converted internally.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift Outdated Show resolved Hide resolved

glbrntt requested changes

View reviewed changes

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift Outdated Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2StreamMultiplexer.swift Outdated Show resolved Hide resolved

johnkassebaum requested review from Lukasa and glbrntt

May 5, 2020 16:13

glbrntt requested changes

View reviewed changes

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Show resolved Hide resolved

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Show resolved Hide resolved

johnkassebaum requested a review from glbrntt

May 5, 2020 17:40

Lukasa requested changes

View reviewed changes

Contributor

Lukasa left a comment

Cool, this looks good. Can we duplicate some tests to actually set the new target window size? It’d be good to have something that validates the existence of the parameter in these two cases.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated

    
                  /// - parameters:

                  ///     - h2ConnectionChannelConfigurator: An optional callback that will be invoked only

                  ///         when the negotiated protocol is H2 to configure the connection channel.

                  ///     - targetWindowSize: The target size of the inbound flow control window.

Contributor

Lukasa May 5, 2020

Can we call out that this is the HTTP/2 flow control window specifically?

johnkassebaum requested a review from Lukasa

May 5, 2020 20:59

Contributor Author

johnkassebaum commented May 5, 2020

Duplicated all tests in the NIOHTTP2Tests that create a pipeline.

Lukasa added the 🆕 semver/minor label

Lukasa added this to the 1.12.0 milestone

Lukasa requested changes

View reviewed changes

Contributor

Lukasa left a comment

Ok I'm generally happy with this, just a few notes.

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated

    
                                                     initialLocalSettings: [HTTP2Setting] = nioDefaultSettings,

                                                     position: ChannelPipeline.Position = .last,

                                                     inboundStreamStateInitializer: ((Channel, HTTP2StreamID) -> EventLoopFuture<Void>)? = nil) -> EventLoopFuture<HTTP2StreamMultiplexer> {

                      return configureHTTP2Pipeline(mode: mode, initialLocalSettings: initialLocalSettings, position: position, targetWindowSize: 65535, inboundStreamStateInitializer: inboundStreamStateInitializer)

Contributor

Lukasa May 7, 2020

Nit: NIO prefers to use the explicit self keyword.

Suggested change

      
                    return configureHTTP2Pipeline(mode: mode, initialLocalSettings: initialLocalSettings, position: position, targetWindowSize: 65535, inboundStreamStateInitializer: inboundStreamStateInitializer)
          
                    return self.configureHTTP2Pipeline(mode: mode, initialLocalSettings: initialLocalSettings, position: position, targetWindowSize: 65535, inboundStreamStateInitializer: inboundStreamStateInitializer)

Contributor Author

johnkassebaum May 7, 2020

No problem

Sources/NIOHTTP2/HTTP2PipelineHelpers.swift Outdated

    
                  public func configureCommonHTTPServerPipeline(

                      h2ConnectionChannelConfigurator: ((Channel) -> EventLoopFuture<Void>)? = nil,

                      _ configurator: @escaping (Channel) -> EventLoopFuture<Void>) -> EventLoopFuture<Void> {

                      return configureCommonHTTPServerPipeline(h2ConnectionChannelConfigurator: h2ConnectionChannelConfigurator, targetWindowSize: 65535, configurator)

Contributor

Lukasa May 7, 2020

Same nit here:

Suggested change

      
                    return configureCommonHTTPServerPipeline(h2ConnectionChannelConfigurator: h2ConnectionChannelConfigurator, targetWindowSize: 65535, configurator)
          
                    return self.configureCommonHTTPServerPipeline(h2ConnectionChannelConfigurator: h2ConnectionChannelConfigurator, targetWindowSize: 65535, configurator)

Contributor Author

johnkassebaum May 7, 2020

ditto

Contributor

glbrntt commented May 7, 2020

@swift-nio-bot test this please

Contributor

glbrntt commented May 7, 2020

@swift-nio-bot add to whitelist

Contributor

Lukasa commented May 7, 2020

@swift-nio-bot test this please

Contributor

Lukasa commented May 7, 2020

Can you run the generate_linux_tests.rb script in the script directory and check in the resulting changes?

Contributor Author

johnkassebaum commented May 7, 2020

@Lukasa If the request to run the generate_linux_tests.rb script was directed to me, then I'm missing something because it generated no output or changes

Contributor

Lukasa commented May 7, 2020

@johnkassebaum CI claims it will generate this diff:

16:20:27 --- a/Tests/NIOHTTP2Tests/ConfiguringPipelineTests+XCTest.swift
16:20:27 +++ b/Tests/NIOHTTP2Tests/ConfiguringPipelineTests+XCTest.swift
16:20:27 @@ -27,10 +27,15 @@ extension ConfiguringPipelineTests {
16:20:27     static var allTests : [(String, (ConfiguringPipelineTests) -> () throws -> Void)] {
16:20:27        return [
16:20:27                  ("testBasicPipelineCommunicates", testBasicPipelineCommunicates),
16:20:27 +                ("testBasicPipelineCommunicatesWithTargetWindowSize", testBasicPipelineCommunicatesWithTargetWindowSize),
16:20:27                  ("testPipelineRespectsPositionRequest", testPipelineRespectsPositionRequest),
16:20:27 +                ("testPipelineRespectsPositionRequestWithTargetWindowSize", testPipelineRespectsPositionRequestWithTargetWindowSize),
16:20:27                  ("testPreambleGetsWrittenOnce", testPreambleGetsWrittenOnce),
16:20:27 +                ("testPreambleGetsWrittenOnceWithTargetWindowSize", testPreambleGetsWrittenOnceWithTargetWindowSize),
16:20:27                  ("testClosingParentChannelClosesStreamChannel", testClosingParentChannelClosesStreamChannel),
16:20:27 +                ("testClosingParentChannelClosesStreamChannelWithTargetWindowSize", testClosingParentChannelClosesStreamChannelWithTargetWindowSize),
16:20:27                  ("testNegotiatedHTTP2BasicPipelineCommunicates", testNegotiatedHTTP2BasicPipelineCommunicates),
16:20:27 +                ("testNegotiatedHTTP2BasicPipelineCommunicatesWithTargetWindowSize", testNegotiatedHTTP2BasicPipelineCommunicatesWithTargetWindowSize),
16:20:27                  ("testNegotiatedHTTP1BasicPipelineCommunicates", testNegotiatedHTTP1BasicPipelineCommunicates),
16:20:27             ]
16:20:27     }


          Make multiplexer's targetWindowSize configurable and use configurabil…

dfd0f72

…ity when creating stream channels.

Motivation:
Infow target window size constrains throughput. The default hardcoded 65535 is low for some use cases and so needs to be configrable. A TODO in the code stated 'Make configurable'.

Modifications:
- In HTTP2StreamMultiplexer, responding to a TODO where stream multiplexer creates a stream channel, created a multiplexr stored property to be used which is set in the default initilaizer.
- In HTTP2PipelineHelpers, added versions of configureHTTP2Pipeline(..) and configureCommonHTTP2ServerPipeline that require an activeTargetWindow arg.

Result:
Clients, such as GRPCClient, can configure pipelines with targetWindowSizes to their liking.

Lukasa reviewed

View reviewed changes

docker/docker-compose.1804.53.yaml Outdated

    
                  image: swift-nio-http2:18.04-5.3

                  build:

                    args:

                      base_image: "swiftlang/swift:nightly-master-bionic"

Contributor

Lukasa May 7, 2020

I presume this change was not supposed to come along?

Contributor Author

johnkassebaum May 7, 2020

Heh. Some confusion on my part it seems around being behind master. I will remove it

Lukasa approved these changes

View reviewed changes

Contributor

Lukasa left a comment

OK, LGTM.

Contributor

Lukasa commented May 7, 2020

@swift-nio-bot test this please

Contributor Author

johnkassebaum commented May 7, 2020

Sorry if this appears as a noob question: When is the appropriate time to hit 'Update branch' button?

Contributor

Lukasa commented May 7, 2020

Whenever suits you: but bear in mind that if you force-push to that branch again you can undo the update.

Contributor Author

johnkassebaum commented May 7, 2020

That's what I observe, and which I just did.


          Merge branch 'master' into jk-expose-target-window-size

11d8168

Contributor

Lukasa commented May 7, 2020

@swift-nio-bot test this please

glbrntt approved these changes

View reviewed changes

Contributor

glbrntt left a comment

Cool, looks good once the CI is green 👍

johnkassebaum mentioned this pull request

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

Merged

Contributor

Lukasa commented May 11, 2020

@swift-nio-bot test this please


          Merge branch 'master' into jk-expose-target-window-size

c9ec334

Contributor

Lukasa commented May 11, 2020

@swift-nio-bot test this please

Contributor

Lukasa commented May 11, 2020

5.3 compiler crash is expected.

Lukasa merged commit 0ca9705 into apple:master

Lukasa added a commit to Lukasa/swift-nio-http2 that referenced this pull request


          Respect target window size on inbound channels.

2071b66

Motivation:

As part of apple#202 we added support for configuring the target window size
of stream channels. That was plumbed through into outbound channels, but
we missed the inbound ones, meaning that this only really worked for
clients, not servers. That hardly seems fair!

Modifications:

Pass target window size to inbound channels too.

Result:

We can control target window size of inbound channels.

Lukasa mentioned this pull request

Respect target window size on inbound channels. #205

Merged

weissi pushed a commit that referenced this pull request


          Respect target window size on inbound channels. (#205)

c8f952d

Motivation:

As part of #202 we added support for configuring the target window size
of stream channels. That was plumbed through into outbound channels, but
we missed the inbound ones, meaning that this only really worked for
clients, not servers. That hardly seems fair!

Modifications:

Pass target window size to inbound channels too.

Result:

We can control target window size of inbound channels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor