Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader` #2517

FranzBusch · 2023-09-13T13:39:44Z

Motivation

With our new NIOAsyncChannel and typed bootstrap APIs we want to be able to let users spell out their pipeline in a typed way. Pipelines can contain handlers that have to make a forking decision such as HTTP upgrading. Our current HTTPServerUpgradeHandler is one of those handlers but it lacks strict typing. To interact nicely with our new typed APIs we need to have a new variant of the HTTPServerUpgradeHandler that can carry type information.

Modification

This PR adds a few things:

A new NIOTypedHTTPServerUpgradeHandler + NIOTypedHTTPServeProtocolUpgrader. I also moved the state handling logic to a separate state machine. I thought about unifying the state machines of the old handler and the new one but they differ in behaviour which makes the state machine more complicated.
A new NIOTypedWebSocketServerUpgrader that conforms to NIOTypedHTTPServerProtocolUpgrader
An overhauled WebSocket server example that fully uses Concurrency.

Result

We now have a way to fully type the server side of HTTP protocol upgrading.

FranzBusch · 2023-09-14T12:28:09Z

@swift-server-bot test this please

glbrntt

I'm only part way through reviewing this but dropping my comments so far.

Could you punt the example client and server into a follow-up PR to make this one a bit smaller?

glbrntt · 2023-09-14T13:03:16Z

Sources/NIOHTTP1/HTTPPipelineSetup.swift

+        withPipeliningAssistance pipelining: Bool = true,
+        withServerUpgrade upgrade: NIOTypedHTTPServerUpgradeConfiguration<UpgradeResult>,
+        withErrorHandling errorHandling: Bool = true,
+        withOutboundHeaderValidation headerValidation: Bool = true,
+        withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init()


I think we should avoid all the withs here as they aren't closures enforcing a lifetime

I agree, but the current APIs use this spelling. Do we want to diverge?

I think we should. While there is value in consistency I think diverging from it to make APIs more idiomatic is justified.

glbrntt · 2023-09-14T13:03:26Z

Sources/NIOHTTP1/HTTPPipelineSetup.swift

+        withPipeliningAssistance pipelining: Bool = true,
+        withServerUpgrade upgrade: NIOTypedHTTPServerUpgradeConfiguration<UpgradeResult>,
+        withErrorHandling errorHandling: Bool = true,
+        withOutboundHeaderValidation headerValidation: Bool = true,
+        withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init()


same here re: with

Sources/NIOHTTP1/HTTPPipelineSetup.swift

Sources/NIOHTTP1/NIOTypedHTTPServerUpgradeHandler.swift

Sources/NIOHTTP1/NIOTypedHTTPServerUpgraderStateMachine.swift

Sources/NIOWebSocket/NIOWebSocketServerUpgrader.swift

Sources/NIOHTTP1/NIOTypedHTTPServerUpgraderStateMachine.swift

Lukasa · 2023-09-18T14:25:33Z

Sources/NIOHTTP1/NIOTypedHTTPServerUpgraderStateMachine.swift

+
+        case finished
+
+        case modifying


We shouldn't need this state: have you validated that we do?

So I was believing that this isn't necessary anymore but from @glbrntt and my recent benchmarking we saw that the compiler can still miss correctly avoiding those CoWs even if everything is inlined. So I am being defensive again until we get switch inout.

Lukasa · 2023-09-18T14:26:16Z

Sources/NIOHTTP1/NIOTypedHTTPServerUpgraderStateMachine.swift

+        case failUpgradePromise
+    }
+
+    @inlinable


We're very inconsistent with the @inlinables here, despite all the actions being @usableFromInline. Why is that?

Just me forgetting it in some places. Added it to everything in the state machine now.

FranzBusch · 2023-09-19T13:18:21Z

5.6 CI failure is due to primary associated type usage. Since we are dropping 5.6 support soon, I will just merge this PR after we dropped it.

glbrntt · 2023-09-19T14:18:03Z

Sources/NIOHTTP1/HTTPPipelineSetup.swift

+        withPipeliningAssistance pipelining: Bool = true,
+        withServerUpgrade upgrade: NIOTypedHTTPServerUpgradeConfiguration<UpgradeResult>,
+        withErrorHandling errorHandling: Bool = true,
+        withOutboundHeaderValidation headerValidation: Bool = true,
+        withEncoderConfiguration encoderConfiguration: HTTPResponseEncoder.Configuration = .init()


I think we should. While there is value in consistency I think diverging from it to make APIs more idiomatic is justified.

glbrntt · 2023-09-21T09:47:49Z

Sources/NIOHTTP1/HTTPPipelineSetup.swift

+        isPipeliningEnabled: Bool = true,
+        isErrorHandlingEnabled: Bool = true,
+        isResponseHeaderValidationEnabled: Bool = true,


I think is isn't right here, because it reads like a question. I think enablePipelining, enableErrorHandling, and enableResponseHeaderValidation are more appropriate

adam-fowler · 2023-09-24T15:23:35Z

I've only had a quick look at this. The websocket upgrade code seems to require that every packet be tested for whether it is an http packet, or an upgraded packet. Can we not do the test on the channel once instead, ie is this channel producing http or websocket packets.

I was hoping the websocket server code could be separate from the http server code but this implementation means the two are intertwined and hard to separate into two modules.

FranzBusch · 2023-09-24T18:32:38Z

I've only had a quick look at this. The websocket upgrade code seems to require that every packet be tested for whether it is an http packet, or an upgraded packet. Can we not do the test on the channel once instead, ie is this channel producing http or websocket packets.

I was hoping the websocket server code could be separate from the http server code but this implementation means the two are intertwined and hard to separate into two modules.

The two are not really intertwined. The NIOTypedHTTPServerUpgrader is only present in the pipeline until the first request comes in. Once the first request is received, we check if it was an upgrade request and if we can handle it. Once, it is either upgraded or not the NIOTypedHTTPServerUpgrader removes itself from the pipeline. Hence, we only really check if the incoming reads are HTTP parts until we have received a complete HTTP request from the peer.

adam-fowler · 2023-09-25T09:23:22Z

I wasn't really talking about the NIOTypedHTTPServerUpgrader. But I misread the sample code and thought you were switching over the packet type (ie websocket packet or http packet) but you are switching over the UpgradeResult enum which holds AsyncChannels of each type. Ignore me.

# Motivation With our new `NIOAsyncChannel` and typed bootstrap APIs we want to be able to let users spell out their pipeline in a typed way. Pipelines can contain handlers that have to make a forking decision such as HTTP upgrading. Our current `HTTPServerUpgradeHandler` is one of those handlers but it lacks strict typing. To interact nicely with our new typed APIs we need to have a new variant of the `HTTPServerUpgradeHandler` that can carry type information. # Modification This PR adds a few things: 1. A new `NIOTypedHTTPServerUpgradeHandler` + `NIOTypedHTTPServeProtocolUpgrader`. I also moved the state handling logic to a separate state machine. I thought about unifying the state machines of the _old_ handler and the new one but they differ in behaviour which makes the state machine more complicated. 2. A new `NIOTypedWebSocketServerUpgrader` that conforms to `NIOTypedHTTPServerProtocolUpgrader` 3. An overhauled WebSocket server example that fully uses Concurrency. # Result We now have a way to fully type the server side of HTTP protocol upgrading. Code review Update parameter names for new API and fix example

FranzBusch · 2023-10-02T11:24:01Z

@Lukasa @glbrntt This PR is now ready to merge. In my last commit, I renamed the methods to configureUpgradableHTTPServerPipeline which aligns closer to what we are doing here. Furthermore, I added a new NIOUpgradableHTTPServerPipelineConfiguration which allows us to extend this configuration method in the future with new defaults without requiring new APIs.

glbrntt

Couple of nits, looks good otherwise.

Sources/NIOHTTP1/HTTPTypedPipelineSetup.swift

glbrntt · 2023-10-02T13:53:13Z

Sources/NIOHTTP1/HTTPTypedPipelineSetup.swift

+        let requestDecoder = ByteToMessageHandler(HTTPRequestDecoder(leftOverBytesStrategy: .forwardBytes))
+
+        var extraHTTPHandlers: [RemovableChannelHandler] = [requestDecoder]
+        extraHTTPHandlers.reserveCapacity(3)


I think you'll need to reserve then add the decoder

glbrntt · 2023-10-02T13:53:34Z

Sources/NIOHTTP1/HTTPTypedPipelineSetup.swift

+    public var enableResponseHeaderValidation = true
+
+    /// The configuration for the ``HTTPResponseEncoder``.
+    public var httpResponseEncoderConfiguration = HTTPResponseEncoder.Configuration()


You missed this one, we can drop http here too

Will fix both in my follow up client PR. Thanks!

# Motivation In my previous PR apple#2517, I added a new typed `HTTPServerUpgrader` and corresponding implementation for the `WebSocketServerUpgrader`. The goal of those is to carry type information across HTTP upgrades which allows us to build fully typed pipelines. # Modification This PR adds a few things: 1. A new `NIOTypedHttpClientUpgradeHandler` + `NIOTypedHttpClientProtocolUpgrader`. I also moved the state handling to a separate state machine. Similar to the server PR I did not unify the state machine between the newly typed and untyped upgrade handlers since they differ in logic. 2. A new `NIOTypedWebSocketClientUpgrader` 3. An overhauled WebSocket client example. # Result This is the last missing piece of dynamic pipeline changing where we did not carry around the type information. After this PR lands, we can finalize the `AsyncChannel` and async typed NIO pieces.

#2526) * Introduce new typed `HTTPClientUpgrader` and `WebSocketClientUpgrader` # Motivation In my previous PR #2517, I added a new typed `HTTPServerUpgrader` and corresponding implementation for the `WebSocketServerUpgrader`. The goal of those is to carry type information across HTTP upgrades which allows us to build fully typed pipelines. # Modification This PR adds a few things: 1. A new `NIOTypedHttpClientUpgradeHandler` + `NIOTypedHttpClientProtocolUpgrader`. I also moved the state handling to a separate state machine. Similar to the server PR I did not unify the state machine between the newly typed and untyped upgrade handlers since they differ in logic. 2. A new `NIOTypedWebSocketClientUpgrader` 3. An overhauled WebSocket client example. # Result This is the last missing piece of dynamic pipeline changing where we did not carry around the type information. After this PR lands, we can finalize the `AsyncChannel` and async typed NIO pieces. * Remove availability on the protocols

FranzBusch requested review from Lukasa and glbrntt September 13, 2023 13:39

glbrntt reviewed Sep 14, 2023

View reviewed changes

glbrntt reviewed Sep 15, 2023

View reviewed changes

Sources/NIOWebSocket/NIOWebSocketServerUpgrader.swift Outdated Show resolved Hide resolved

Sources/NIOWebSocket/NIOWebSocketServerUpgrader.swift Outdated Show resolved Hide resolved

Lukasa reviewed Sep 18, 2023

View reviewed changes

FranzBusch requested review from Lukasa and glbrntt September 19, 2023 13:18

glbrntt approved these changes Sep 19, 2023

View reviewed changes

glbrntt reviewed Sep 21, 2023

View reviewed changes

FranzBusch added 2 commits October 2, 2023 12:22

Introduce new configuration struct and rename to UpgradablePipeline

68c64df

FranzBusch force-pushed the fb-websocket-server-upgrade branch from d126c9f to 68c64df Compare October 2, 2023 11:22

FranzBusch requested a review from glbrntt October 2, 2023 11:24

glbrntt reviewed Oct 2, 2023

View reviewed changes

FranzBusch enabled auto-merge (squash) October 2, 2023 13:47

glbrntt reviewed Oct 2, 2023

View reviewed changes

Review comments

4da9b3a

FranzBusch force-pushed the fb-websocket-server-upgrade branch from 3a5392a to 4da9b3a Compare October 2, 2023 14:00

FranzBusch merged commit de07e57 into apple:main Oct 2, 2023
6 of 8 checks passed

FranzBusch deleted the fb-websocket-server-upgrade branch October 2, 2023 14:26

FranzBusch mentioned this pull request Oct 2, 2023

Introduce new typed HTTPClientUpgrader and WebSocketClientUpgrader #2526

Merged

FranzBusch added the semver/minor Adds new public API. label Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader` #2517

Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader` #2517

FranzBusch commented Sep 13, 2023

FranzBusch commented Sep 14, 2023

glbrntt left a comment

glbrntt Sep 14, 2023

FranzBusch Sep 19, 2023

glbrntt Sep 19, 2023

glbrntt Sep 14, 2023

Lukasa Sep 18, 2023

FranzBusch Sep 19, 2023

Lukasa Sep 18, 2023

FranzBusch Sep 19, 2023

FranzBusch commented Sep 19, 2023

glbrntt Sep 19, 2023

glbrntt Sep 21, 2023

adam-fowler commented Sep 24, 2023

FranzBusch commented Sep 24, 2023

adam-fowler commented Sep 25, 2023

FranzBusch commented Oct 2, 2023

glbrntt left a comment

glbrntt Oct 2, 2023

glbrntt Oct 2, 2023

FranzBusch Oct 2, 2023

Introduce new typed HTTPServerUpgrader and WebSocketServerUpgrader #2517

Introduce new typed HTTPServerUpgrader and WebSocketServerUpgrader #2517

Conversation

FranzBusch commented Sep 13, 2023

Motivation

Modification

Result

FranzBusch commented Sep 14, 2023

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-fowler commented Sep 24, 2023

FranzBusch commented Sep 24, 2023

adam-fowler commented Sep 25, 2023

FranzBusch commented Oct 2, 2023

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader` #2517

Introduce new typed `HTTPServerUpgrader` and `WebSocketServerUpgrader` #2517