[HTTP2ConnectionPool] added `HTTP2Connections` struct #440

dnadoba · 2021-09-27T09:11:40Z

One step closer to support HTTP/2 in the new connection pool.
HTTP2Connections will be used in a new HTTP2StateMaschine in a follow up PR.

fabianfett

Looks great. Small stuff now.

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

fabianfett · 2021-09-27T10:23:13Z

5.5 failed because of an unrelated issue:

Test Case 'HTTPClientTests.testDelegateCallinsTolerateRandomEL' started at 2021-09-27 09:19:39.047
AsyncHTTPClient/RequestBag+StateMachine.swift:476: Fatal error: Invalid state... If no error occured, this must not be called, after the request was finished
Current stack trace:
0    libswiftCore.so                    0x00007fa00576da10 swift_reportError + 50
1    libswiftCore.so                    0x00007fa0057e61a0 _swift_stdlib_reportFatalErrorInFile + 109
2    libswiftCore.so                    0x00007fa0054f34d2 <unavailable> + 1414354
3    libswiftCore.so                    0x00007fa0054f31fb <unavailable> + 1413627
4    libswiftCore.so                    0x00007fa0054f1ee0 _assertionFailure(_:_:file:line:flags:) + 447
5    async-http-clientPackageTests.xctest 0x0000560e5eb93d77 <unavailable> + 1871223
6    async-http-clientPackageTests.xctest 0x0000560e5eb92eb4 <unavailable> + 1867444
7    async-http-clientPackageTests.xctest 0x0000560e5eba5403 <unavailable> + 1942531
8    async-http-clientPackageTests.xctest 0x0000560e5eba603b <unavailable> + 1945659
9    async-http-clientPackageTests.xctest 0x0000560e5ea998f9 <unavailable> + 846073
10   async-http-clientPackageTests.xctest 0x0000560e5f11ad6c <unavailable> + 7667052
11   async-http-clientPackageTests.xctest 0x0000560e5f113a81 <unavailable> + 7637633
12   async-http-clientPackageTests.xctest 0x0000560e5f114b2e <unavailable> + 7641902
13   async-http-clientPackageTests.xctest 0x0000560e5f1bc2bc <unavailable> + 8327868
14   async-http-clientPackageTests.xctest 0x0000560e5f3c6da1 <unavailable> + 10468769
15   async-http-clientPackageTests.xctest 0x0000560e5f1b9f2c <unavailable> + 8318764
16   async-http-clientPackageTests.xctest 0x0000560e5f3c2a22 <unavailable> + 10451490
17   async-http-clientPackageTests.xctest 0x0000560e5eb4be7f <unavailable> + 1576575
18   async-http-clientPackageTests.xctest 0x0000560e5f3c56e4 <unavailable> + 10462948
19   async-http-clientPackageTests.xctest 0x0000560e5f3bcc02 <unavailable> + 10427394
20   async-http-clientPackageTests.xctest 0x0000560e5f3c10f0 <unavailable> + 10445040
21   async-http-clientPackageTests.xctest 0x0000560e5f38b1a1 <unavailable> + 10224033
22   async-http-clientPackageTests.xctest 0x0000560e5f38b84e <unavailable> + 10225742
23   async-http-clientPackageTests.xctest 0x0000560e5f390a23 <unavailable> + 10246691
24   async-http-clientPackageTests.xctest 0x0000560e5f389dbf <unavailable> + 10218943
25   async-http-clientPackageTests.xctest 0x0000560e5f3f9831 <unavailable> + 10676273
26   async-http-clientPackageTests.xctest 0x0000560e5f3fc3b2 <unavailable> + 10687410
27   async-http-clientPackageTests.xctest 0x0000560e5f3fc519 <unavailable> + 10687769
28   libpthread.so.0                    0x00007fa0051d7609 <unavailable> + 38409
29   libc.so.6                          0x00007fa0045e4250 clone + 67

@fabianfett investigates: https://ci.swiftserver.group/job/async-http-client-swift55-prb/154/console

fabianfett · 2021-09-27T13:12:57Z

5.5 fails because of an issue that is fixed in #441.

fabianfett

Looks great! @Lukasa would you mind giving this another look?

fabianfett · 2021-09-27T13:23:39Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+            if self.isIdle {
+                stats.idleConnections &+= 1
+            }


NIT: maybe move this into .active and check if usedStream == 0.

Lukasa · 2021-09-27T14:17:24Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+        private enum State {
+            /// the pool is establishing a connection. Valid transitions are to: .backingOff, .active and .closed
+            case starting
+            /// the connection is waiting to retry to establish a connection. Transition to .closed. From .closed


Presumably this wanted to say "Valid transitions are to:"?

Lukasa · 2021-09-27T14:18:16Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+            case closed
+        }
+
+        var isActive: Bool {


It might be useful to clarify the intended meaning of "active" here: it's a bit of a surprise to me that .draining connections aren't active.

We are actually no longer using isActive and I have now removed it.

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

Lukasa · 2021-09-27T14:50:29Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+                    lastIdle = .now()
+                }
+                self.state = .active(conn, maxStreams: maxStreams, usedStreams: usedStreams, lastIdle: lastIdle)
+                return max(maxStreams - usedStreams, 0)


This max call is unnecessary: we can raise the assert above into a precondition and then just return maxStreams - usedStreams.

Additionally, while we're here, the same precondition means we can use unchecked subtraction.

I think max actually is necessary because we could receive a new max concurrent streams settings at any time which is lower than the current streams in use. In that case, we would potentially report a negative number of available streams.

Nevertheless, I agree with raising the assert to a precondition and the unchecked subtraction.

Lukasa · 2021-09-27T14:50:37Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+
+            case .draining(let conn, let maxStreams, var usedStreams):
+                usedStreams -= 1
+                assert(usedStreams >= 0)


Let's make this a precondition.

Lukasa · 2021-09-27T15:10:22Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+                return .removeConnection
+
+            case .active(let connection, _, let usedStreams, _):
+                if usedStreams <= 0 {


Might be better to precondition on this number being less than zero.

Lukasa · 2021-09-27T15:12:25Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+            return connection.connectionID
+        }
+
+        /// A new HTTP/2.0 connection was established.


Giant nitpick: It's HTTP/2, not HTTP/2.0. No minor version.

Lukasa · 2021-09-27T15:14:23Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift

+        /// tries to find an available connection on the prefered `eventLoop`. If it can't find one with the given `eventLoop`, it returns the first available connection
+        private func findAvailableConnection(onPreferred eventLoop: EventLoop) -> Int? {
+            var availableConnection: Int?
+            for (index, connection) in self.connections.enumerated() {


We should be somewhat cautious here: enumerated does not return an (index, element) tuple, it returns an (offset, element) tuple. We should probably write a version of this that does what you actually want, in case we ever change the underlying object.

dnadoba · 2021-09-28T08:26:47Z

swift-nightly fails because of Sendable requirements.

dnadoba force-pushed the dn-http2-connections-struct branch from 5e83e02 to 2b3b29a Compare September 27, 2021 09:12

fabianfett reviewed Sep 27, 2021

View reviewed changes

fabianfett requested a review from Lukasa September 27, 2021 10:19

dnadoba marked this pull request as ready for review September 27, 2021 12:10

dnadoba force-pushed the dn-http2-connections-struct branch from 4de065c to 1289735 Compare September 27, 2021 13:03

fabianfett approved these changes Sep 27, 2021

View reviewed changes

dnadoba force-pushed the dn-http2-connections-struct branch from 1289735 to dfd000d Compare September 27, 2021 14:03

Lukasa reviewed Sep 27, 2021

View reviewed changes

fabianfett added this to the HTTP/2 support milestone Sep 27, 2021

fabianfett added the 🔨 semver/patch No public API change. label Sep 27, 2021

dnadoba requested a review from Lukasa September 28, 2021 07:59

dnadoba added 5 commits September 28, 2021 10:09

[HTTP2ConnectionPool] added HTTP2Connections struct

609e533

fix review comments from @fabianfett

4d1759c

fix review comments from @Lukasa

38154d2

use index(_:offsetBy:) to convert from offset to index

237494e

draining connections do not count as idle connections

88f3606

dnadoba force-pushed the dn-http2-connections-struct branch from eb5cd0a to 88f3606 Compare September 28, 2021 08:15

Lukasa approved these changes Sep 28, 2021

View reviewed changes

dnadoba merged commit 88d47f1 into swift-server:main Sep 28, 2021

dnadoba deleted the dn-http2-connections-struct branch October 12, 2021 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP2ConnectionPool] added `HTTP2Connections` struct #440

[HTTP2ConnectionPool] added `HTTP2Connections` struct #440

dnadoba commented Sep 27, 2021

fabianfett left a comment

fabianfett commented Sep 27, 2021

fabianfett commented Sep 27, 2021

fabianfett left a comment

fabianfett Sep 27, 2021

Lukasa Sep 27, 2021

Lukasa Sep 27, 2021

dnadoba Sep 27, 2021

Lukasa Sep 27, 2021

dnadoba Sep 27, 2021 •

edited

Loading

Lukasa Sep 27, 2021

Lukasa Sep 27, 2021

Lukasa Sep 27, 2021

Lukasa Sep 27, 2021

dnadoba commented Sep 28, 2021

[HTTP2ConnectionPool] added HTTP2Connections struct #440

[HTTP2ConnectionPool] added HTTP2Connections struct #440

Conversation

dnadoba commented Sep 27, 2021

fabianfett left a comment

Choose a reason for hiding this comment

fabianfett commented Sep 27, 2021

fabianfett commented Sep 27, 2021

fabianfett 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

dnadoba Sep 27, 2021 • edited Loading

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

dnadoba commented Sep 28, 2021

[HTTP2ConnectionPool] added `HTTP2Connections` struct #440

[HTTP2ConnectionPool] added `HTTP2Connections` struct #440

dnadoba Sep 27, 2021 •

edited

Loading