Add no-op NIOHTTP2ConnectionDemultiplexer #377
Conversation
| @@ -272,6 +278,7 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler { | |||
| // This is the easy one. We were active, now we aren't. | |||
| self.activationState = .inactive | |||
| self.channelClosed = true | |||
| self.demultiplexer = nil | |||
There was a problem hiding this comment.
This is a bit aggressive: we don't need to throw this away right now.
|
|
||
| /// Wraps user event types. | ||
| /// | ||
| /// This allows us to buffer and pass around the events without making use of a generic. |
There was a problem hiding this comment.
Strictly this was an existential.
|
|
||
| func pendingUserEvent(_ event: Any) { | ||
| func pendingUserEvent(_ event: BufferedHTTP2UserEvent) { |
There was a problem hiding this comment.
To reduce the noise of this patch somewhat, it might be nicer to add individual pendingUserEvent functions for each of the types in the enum cases, and then the call sites don't have to change.
| self.demultiplexer?.streamWindowUpdated(event.streamID, inboundWindowSize: event.inboundWindowSize, outboundWindowSize: event.outboundWindowSize) | ||
| case .streamClosed(let event): | ||
| self.demultiplexer?.streamClosed(event.streamID, reason: event.reason) | ||
| } |
There was a problem hiding this comment.
Can we hide this switch in an extension on the demultiplexer? It'll again let us clean up this call site somewhat.
There was a problem hiding this comment.
I have moved this to:
extension NIOHTTP2Handler.ConnectionDemultiplexer {
func process(event: InboundEventBuffer.BufferedHTTP2UserEvent) {
Motivation: This change prepares for future work to embed the connection demultiplexer within the `HTTP2ChannelHandler` which will improve performance by removing the need for passing expensive `InboundUserEvent`s. Modifications: This first step defines a new protocol (`NIOHTTP2ConnectionDemultiplexer`) which will later be used to abstract over the embedded and legacy separate demultiplexer code but at the moment should not change the logic. Result: Behaviour should be unchanged.
* no longer `nil` out demultiplexer on channelInactive * restructure calling `pendingUserEvent` & processing events
rnro
force-pushed
the
add_demultiplexer_protocol_abstraction_to_NIOHTTP2Handler
branch
from
74741be
to
bfe93ce
Compare
| self.inboundEventBuffer.pendingUserEvent( | ||
| NIOHTTP2StreamCreatedEvent(streamID: streamCreatedData.streamID, | ||
| localInitialWindowSize: streamCreatedData.localStreamWindowSize.map(UInt32.init), | ||
| remoteInitialWindowSize: streamCreatedData.remoteStreamWindowSize.map(UInt32.init))) |
There was a problem hiding this comment.
Can you clean up the indentation here?
There was a problem hiding this comment.
I think I have done this now. I have certainly changed it.
| case .streamWindowUpdated(let event): | ||
| self.streamWindowUpdated(event.streamID, inboundWindowSize: event.inboundWindowSize, outboundWindowSize: event.outboundWindowSize) | ||
| case .streamClosed(let event): | ||
| self.streamClosed(event.streamID, reason: event.reason) |
There was a problem hiding this comment.
Seeing it like this makes me ask the question: is there a good reason these functions aren't defined to simply take the event directly?
There was a problem hiding this comment.
When I sketched out the API I didn't consider what was already there (i.e. the inbound event buffer). So no, no good reason to not use events directly.
There was a problem hiding this comment.
Changed to use the events directly.
NIOHTTP2ConnectionDemultiplexer stream events now use event types directly rather than individual parameters which map to the event fields.
Motivation:
This change prepares for future work to embed the connection demultiplexer within the
HTTP2ChannelHandlerwhich will improve performance by removing the need for passing expensiveInboundUserEvents.Modifications:
This first step defines a new protocol (
NIOHTTP2ConnectionDemultiplexer) which will later be used to abstract over the embedded and legacy separate demultiplexer code but at the moment should not change the logic.Result:
Behaviour should be unchanged.