Conversation
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Could you add the unmodified code from gRPC as the first commit in this PR to make it easier to see the changes you've made?
aryan-25
force-pushed
the
server-connection-manager
branch
from
edf43a7 to
ce91534
Compare
| /// While NIO's `EmbeddedEventLoop` provides control over its view of time (and therefore any | ||
| /// events scheduled on it) it doesn't offer a way to get the current time. This is usually done | ||
| /// via `NIODeadline`. | ||
| public struct Clock { |
There was a problem hiding this comment.
I would err on the side of caution and keep this internal/package because it was the minimal API required to enable various tests in gRPC. We can always think a bit harder about the API and make it public in the future if necessary.
| /// - keepaliveConfiguration: Configuration for keep-alive ping behavior. Defaults to `nil`, disabling keep-alive | ||
| /// pings. | ||
| /// - clock: A clock providing the current time. | ||
| public init( |
There was a problem hiding this comment.
I recommend adding a config struct for all of these values: it will make API evolution much easier (e.g. if more config is added later). Also means you can provide a set of sensible default values as a static computed property on that config too.
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | ||
| public func configureAsyncHTTP2Pipeline<Output: Sendable>( | ||
| mode: NIOHTTP2Handler.ParserMode, | ||
| connectionManager: NIOHTTP2ServerConnectionManagementHandler, |
There was a problem hiding this comment.
Passing in the handler is a bit odd. It'd be more idiomatic to pass in the config for the handler and have this func create the handler for the caller.
| } | ||
| } | ||
|
|
||
| extension Timer: NIOScheduledCallbackHandler, @unchecked Sendable where Handler: Sendable { |
There was a problem hiding this comment.
This unchecked sendable seems wrong: it relies on correct use. Can we make it actually Sendable?
| } | ||
|
|
||
| /// A manual clock for testing that allows explicit control over time. | ||
| public final class Manual { |
There was a problem hiding this comment.
These publics don’t do anything, so we can safely remove them.
| } | ||
|
|
||
| public func handlerAdded(context: ChannelHandlerContext) { | ||
| assert(context.eventLoop === self.eventLoop) |
There was a problem hiding this comment.
Replace this assert with an in-event-loop assertion.
|
|
||
| // Timer handler views. | ||
| extension NIOHTTP2ServerConnectionManagementHandler { | ||
| struct MaxIdleTimerHandlerView: @unchecked Sendable, NIOScheduledCallbackHandler { |
There was a problem hiding this comment.
What’s the need for these to be Sendable? If we do actually require it, we should fully enforce it using proper preconditions or loop bounds.
|
|
||
| extension NIOHTTP2ServerConnectionManagementHandler { | ||
| /// A delegate for receiving HTTP/2 stream lifecycle events. | ||
| public struct HTTP2StreamDelegate: @unchecked Sendable, NIOHTTP2StreamDelegate { |
There was a problem hiding this comment.
Why is this public? Same note here re sendability as well.
3 participants
Motivation:
NIOHTTP2Handlercurrently does not support graceful shutdown because it does not action upon aChannelShouldQuiesceEventevent. See issue #336.Per the comment on that issue,
gRPC-swift-nio-transportimplements this functionality in a separate channel handler, namedServerConnectionManagementHandler. We should introduce this to this repo too.Modifications:
ServerConnectionManagementHandlerfromgRPC-swift-nio-transportwith some slight modifications such as removing gRPC-specific functionality like policing client pings.configureAsyncHTTP2Pipelinehelper, which takes the connection manager handler as an argument and sets it up alongsideNIOHTTP2Handler.Result:
Addresses issue #336.