Repository navigation
Conversation
31649a1 to
e32137f
Compare
| /// Only HTTP/3 connection channels have one: a QUIC connection can migrate to a new network path, whereas a TCP | ||
| /// connection's path is fixed for its lifetime. | ||
| @available(anyAppleOS 27.0, *) | ||
| final class ConnectionEventsHandler: ChannelInboundHandler, RemovableChannelHandler { |
There was a problem hiding this comment.
We can reuse HTTP3ConnectionManager here instead of defining a new handler.
| self.currentRemoteAddress = NIOLockedValueBox(remoteAddress) | ||
| self.localAddress = localAddress | ||
| self.validatedPeerCertificateChain = validatedPeerCertificateChain | ||
| self.connectionEvents = ConnectionEvents(broadcaster: ConnectionEventBroadcaster()) |
There was a problem hiding this comment.
We should avoid initialising this over HTTP/1.1 and HTTP/2.
| /// This is what lets several handlers iterate a connection's events concurrently: the iterators of a single | ||
| /// `AsyncStream` would instead compete for its elements, with each event reaching only one of them. | ||
| @available(anyAppleOS 27.0, *) | ||
| final class ConnectionEventBroadcaster: Sendable { |
There was a problem hiding this comment.
Wondering if it might be better to broadcast connection events to streams at the swift-nio-http3 layer.
HTTP3ConnectionCoordinator holds a reference to all streams.
In apple/swift-nio-http3#72 for example, this property is used to inform all open outbound request streams about whether the server supports Extended CONNECT (the client only finds out once the server sends their SETTINGS).
There was a problem hiding this comment.
If we went this route, we'd have to do this at the h2 layer too though right?
Not saying we should or shouldn't, I'm still undecided. But I'm not sure where the responsibility lies really. This is a connection-level event after all. Not sure broadcasting it to all streams would make sense.
| /// } | ||
| /// } | ||
| /// ``` | ||
| public struct ConnectionEvents: AsyncSequence, Sendable { |
There was a problem hiding this comment.
I am not a big fan of modeling this with an asynchronous sequence. I would rather like to see this modeled as:
withConnectionEventHandler {
// handle request
} onEvent { event in
}The benefit of this is that it can be implemented a lot simpler than an asynchronous sequences. In particular, we don't have to buffer events. It makes the entire surface below with the broadcaster also go away. Users can still create asynchronous sequences out of the events themselves.
There was a problem hiding this comment.
Would you call this from within the request handler? How does it make the broadcaster go away? We still need to translate connection channel events into something that is received by all requests being handled on that connection, right?
There was a problem hiding this comment.
Yes inside a request handler you would do the above. This follows the structured eventing pattern of withTaskCancellationHandler and withGracefulShutdown handler.
We need something similar to the GracefulShutdownManager of swift-service-lifecycle but we don't need to broadcast. It just stores the handler closures.
3 participants
Summary
This PR adds support for network path change notifications, via an
HTTPServerCapabilitythat the connection context conforms to.Modifications
NIOHTTPServer.ConnectionEvents, a multicastAsyncSequencethat finishes when the connection closes. It's available onConnectionContextand, through the newHTTPServerCapability.ConnectionEvents, onRequestContext.ConnectionContext.remoteAddressnow always reflects the current address, as it previously did not react to network path changes. This is done via aConnectionEventsHandleron every H3 connection channel, which updates the context from pipeline events and broadcasts them. H1 and H2 connections can't change paths, so the handler is omitted on those pipelines.Note: network path change events are not yet present in
swift-nio-quic, so the event present in this change is only a placeholder for now.