Add HTTP2Connection #401

fabianfett · 2021-07-08T15:00:23Z

This pr adds an HTTP2Connection that we want to use in our connection pool going forward.

It has no tests at all. I would like to get some early feedback before adding those.

Lukasa · 2021-07-08T16:42:36Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+
+    private(set) var channelContext: ChannelHandlerContext?
+    private(set) var state: HTTPRequestStateMachine = .init(isChannelWritable: false)
+    private(set) var request: HTTPExecutingRequest?


Any reason for these not to be fully private?

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

Lukasa · 2021-07-08T16:48:08Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+
+        try sync.addHandler(http2Handler, position: .last)
+        try sync.addHandler(idleHandler, position: .last)
+        try sync.addHandler(self.multiplexer, position: .last)


Why are we building the HTTP/2 handlers ourselves?

Because we need to add the IdleHandler in the middle. This trick was imported from grpc based on @glbrntt's suggestion.

Can we add a comment to that effect please?

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

glbrntt · 2021-07-08T16:41:46Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

+
+            let maxStream = settings.first(where: { $0.parameter == .maxConcurrentStreams })?.value ?? 100
+
+            self.state = .active(openStreams: 0, maxStreams: maxStream)


A SETTINGS frame MUST be sent by both endpoints at the start of a connection and MAY be sent at any other time by either endpoint over the lifetime of the connection.

We need to handle this otherwise we'll blow away our open stream count. Moreover we can't always assume 100 as the default when no setting is present (since a mid-connection settings update may change another setting and we don't want to inadvertently overwrite the previous value of max concurrent streams).

glbrntt · 2021-07-08T16:48:18Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

+                self.state = .goAwayReceived(openStreams: openStreams, maxStreams: maxStreams)
+                return .notifyConnectionGoAwayReceived
+            case .goAwayReceived:
+                preconditionFailure("Invalid state")


IIRC it's valid for a server to send more than one GOAWAY frame.

Lukasa · 2021-07-12T10:44:24Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+    func http2ConnectionClosed(_: HTTP2Connection)
+}
+
+class HTTP2Connection {


Any reason this shouldn't be final?

Lukasa · 2021-07-12T10:44:48Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+
+        try sync.addHandler(http2Handler, position: .last)
+        try sync.addHandler(idleHandler, position: .last)
+        try sync.addHandler(self.multiplexer, position: .last)


Can we add a comment to that effect please?

Lukasa · 2021-07-12T10:46:04Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+        self.state = .starting(readyToAcceptConnectionsPromise)
+        self.readyToAcceptConnectionsFuture = readyToAcceptConnectionsPromise.futureResult
+
+        // 1. Modify channel pipeline and add http2 handlers


It's not clear to me that we should do this on init. I tend to be a bit nervous about inits that do a lot of work. Would it be better to move this to a, say, start method?

Lukasa · 2021-07-12T10:46:45Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+        self.multiplexer = HTTP2StreamMultiplexer(
+            mode: .client,
+            channel: channel,
+            targetWindowSize: 65535,


We should almost certainly default this to a substantially larger number. 65535 is the protocol default and it's very conservative.

I'd normally suggest something closer to 8MB.

glbrntt · 2021-07-19T10:34:28Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+import NIOHTTP1
+import NIOHTTP2
+
+class HTTP2ClientRequestHandler: ChannelDuplexHandler {


glbrntt · 2021-07-19T10:37:57Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+            if let newRequest = self.request {
+                if let idleReadTimeout = newRequest.idleReadTimeout {
+                    self.idleReadTimeoutStateMachine = .init(timeAmount: idleReadTimeout)
+                }


What are we doing with the idleReadTimeoutStateMachine if newRequest.idleReadTimeout == nil? Should the two if-lets be a single line?

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

glbrntt · 2021-07-19T10:49:06Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+    // MARK: Private HTTPRequestExecutor
+
+    private func writeRequestBodyPart0(_ data: IOData, request: HTTPExecutableRequest) {
+        guard self.request === request, let context = self.channelContext else {


super nit: we repeat this a handful of times, might be worth extracting it.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

glbrntt

I didn't get a chance to look at the tests yet but leaving these comments for the time being.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

glbrntt · 2021-07-22T09:14:38Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

+            case .closing(var openStreams, let maxStreams):
+                openStreams += 1


In what situations can we open a stream when we're closing?

glbrntt · 2021-07-22T09:16:05Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift


-            openStreams -= 1
-            assert(openStreams >= 0)


Why did we drop the assertion? It seems relevant at least in the active case.

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Factory.swift

Co-authored-by: George Barnett <[email protected]>

Lukasa · 2021-07-22T15:03:35Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+    private let eventLoop: EventLoop
+
+    private var state: HTTPRequestStateMachine = .init(isChannelWritable: false) {
+        didSet {


This should be willSet, or this won't fire early enough.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

Lukasa · 2021-07-22T15:10:44Z

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+        }
+
+        static func == (lhs: Self, rhs: Self) -> Bool {
+            lhs.channel === rhs.channel


Make the equatable/hashable implementations the same.

fabianfett added the 🔨 semver/patch No public API change. label Jul 8, 2021

fabianfett added this to the HTTP/2 support milestone Jul 8, 2021

fabianfett requested review from Lukasa and glbrntt July 8, 2021 15:00

fabianfett force-pushed the ff-h2-connection branch 2 times, most recently from d5ba480 to f507cdd Compare July 8, 2021 16:27

Lukasa reviewed Jul 8, 2021

View reviewed changes

glbrntt reviewed Jul 8, 2021

View reviewed changes

fabianfett force-pushed the ff-h2-connection branch 3 times, most recently from 5c7899a to 5376b92 Compare July 10, 2021 12:16

Lukasa reviewed Jul 12, 2021

View reviewed changes

fabianfett added 5 commits July 13, 2021 13:36

Add HTTP2Connection

375b6e7

Code review

c18242f

Rebase fixes

c08452c

Added TODO

e08bd92

Some HTTP1 cleanup

b8a266c

fabianfett force-pushed the ff-h2-connection branch from 5376b92 to deaf5b4 Compare July 16, 2021 16:54

Better HTTP/2

0bac419

fabianfett force-pushed the ff-h2-connection branch from deaf5b4 to 0bac419 Compare July 16, 2021 17:50

fabianfett requested a review from glbrntt July 19, 2021 08:38

glbrntt reviewed Jul 19, 2021

View reviewed changes

fabianfett requested a review from glbrntt July 20, 2021 16:13

Code review part 3

4af1b1a

fabianfett force-pushed the ff-h2-connection branch from 975c8a0 to 4af1b1a Compare July 20, 2021 16:20

fabianfett added 2 commits July 21, 2021 16:08

Tests

62b7fcc

test less flaky

ae52491

fabianfett requested a review from Lukasa July 21, 2021 14:25

glbrntt reviewed Jul 22, 2021

View reviewed changes

Apply suggestions from code review

e86dcc9

Co-authored-by: George Barnett <[email protected]>

code review

7d6c089

fabianfett force-pushed the ff-h2-connection branch from 69f95a5 to 7d6c089 Compare July 22, 2021 13:59

fabianfett requested a review from glbrntt July 22, 2021 14:18

Lukasa reviewed Jul 22, 2021

View reviewed changes

Code review

df942d0

fabianfett requested a review from Lukasa July 22, 2021 15:49

Lukasa approved these changes Jul 22, 2021

View reviewed changes

fabianfett merged commit 388b8dc into swift-server:main Jul 22, 2021

fabianfett deleted the ff-h2-connection branch July 22, 2021 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTP2Connection #401

Add HTTP2Connection #401

fabianfett commented Jul 8, 2021

Lukasa Jul 8, 2021

Lukasa Jul 8, 2021

fabianfett Jul 8, 2021

Lukasa Jul 12, 2021

glbrntt Jul 8, 2021

glbrntt Jul 8, 2021

Lukasa Jul 12, 2021

Lukasa Jul 12, 2021

Lukasa Jul 12, 2021

Lukasa Jul 12, 2021

Lukasa Jul 12, 2021

glbrntt Jul 19, 2021

glbrntt Jul 19, 2021

glbrntt Jul 19, 2021

glbrntt left a comment

glbrntt Jul 22, 2021

glbrntt Jul 22, 2021

Lukasa Jul 22, 2021

Lukasa Jul 22, 2021


		let maxStream = settings.first(where: { $0.parameter == .maxConcurrentStreams })?.value ?? 100

		self.state = .active(openStreams: 0, maxStreams: maxStream)

		case .closing(var openStreams, let maxStreams):
		openStreams += 1

Add HTTP2Connection #401

Add HTTP2Connection #401

Conversation

fabianfett commented Jul 8, 2021

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

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

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