Add setting for max number of concurrent streams for HTTP/2 connection #1226
Conversation
|
|
…n to circumvent the limitation of a maximum of 100 concurrent calls as imposed by the default settings of NIOHTTP2Handler
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Thanks for the PR, this broadly looks good! I left a few notes inline but I think we'll also want a test for this. It could do something like:
- start a server setting max concurrent streams to N (where N > 100)
- connect to the server
- start N client streaming (or bidirectional streaming) RPCs
- send a message on each RPC (but do not send end)
- send end on RPC N
- wait for the status on RPC N (i.e. if this RPC does not complete then we didn't apply the setting correctly)
- send on all other RPCs, wait for them to complete
Sources/GRPC/ServerBuilder.swift
Outdated
| @discardableResult | ||
| public func with(httpMaxConcurrentStreams: Int) -> Self { | ||
| self.configuration.httpMaxConcurrentStreams = httpMaxConcurrentStreams | ||
| return self | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this be moved down the file next to withHTTPTargetWindowSize(_:) and be named in a similar way?
Sources/GRPC/Server.swift
Outdated
| connectionIdleTimeout: TimeAmount = .nanoseconds(.max), | ||
| messageEncoding: ServerMessageEncoding = .disabled, | ||
| httpTargetWindowSize: Int = 65535, | ||
| httpMaxConcurrentStreams: Int = 100, |
There was a problem hiding this comment.
We can't add this here since it breaks public API -- moreover this init is deprecated anyway (It was deprecated such that we no longer have to deprecate and add new inits every time we add a new property to the configuration).
Sources/GRPC/Server.swift
Outdated
| public var httpTargetWindowSize: Int = 65535 | ||
|
|
||
| /// The HTTP/2 max number of concurrent streams. Defaults to 100. | ||
| public var httpMaxConcurrentStreams: Int = 100 |
There was a problem hiding this comment.
Can we add a precondition in willSet that the new value must be >= 0? (And document the requirement)
Sources/GRPC/ClientConnection.swift
Outdated
| /// The HTTP/2 flow control target window size. Defaults to 65535. | ||
| public var httpTargetWindowSize = 65535 | ||
|
|
||
| public var httpMaxConcurrentStreams: Int = 100 |
There was a problem hiding this comment.
We actually don't need this setting on the client configuration: the setting indicates to the other side how many streams they may create concurrently. I.e. here we're configuring how man the streams the server may create but for gRPC all streams are created by the client.
…r maxConcurrentStreams; Refactor
|
@glbrntt awesome, thanks for your comments, let me know what you think about my changes |
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
This looks great! We're just missing a bit of resource cleanup in the tests then this should be good to go.
2 participants
... in order to circumvent the limitation of a maximum of 100 concurrent calls as imposed by the default settings of NIOHTTP2Handler.