Add new HTTPConnectionPool #418

fabianfett · 2021-09-08T22:24:27Z

Follow up to #416

Lukasa · 2021-09-09T14:55:03Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        case .executeRequest(let request, let connection, cancelTimeout: let cancelTimeout):
+            connection.executeRequest(request.req)
+            if cancelTimeout {
+                self.cancelRequestTimeout(request.id)


Should these two operations be in the other order?

Added a code comment.

I think I disagree with your code comment. If we're about to execute a request then we should aim to cancel the timeout first, as this will make some small number of requests succeed where the other order would fail.

Let's look at the definition of the request timeouts.

When a request timeout fires, it will tell the state machine about it having fired. The state machine will then look for the request in its own request queue. If it doesn't find the request in there, it knows that the request was already removed (probably in another thread) before. The action will for this reason be none.

For this reason the order in calling the request actions does not matter. The only place where we make decisions about failing requests is in the state machine.

From the HTTPConnectionPool:

private func scheduleRequestTimeout(_ request: Request, on eventLoop: EventLoop) { let requestID = request.id let scheduled = eventLoop.scheduleTask(deadline: request.connectionDeadline) { // The timer has fired. Now we need to do a couple of things: // // 1. Remove ourselves from the timer dictionary to not leak any data. If our // waiter entry still exists, we need to tell the state machine, that we want // to fail the request. let timeoutFired = self.timerLock.withLock { self._requestTimer.removeValue(forKey: requestID) != nil } // 2. If the entry did not exists anymore, we can assume that the request was // scheduled on another connection. The timer still fired anyhow because of a // race. In such a situation we don't need to do anything. guard timeoutFired else { return } // 3. Tell the state machine about the timeout let action = self.stateLock.withLock { self._state.timeoutRequest(requestID) } self.run(action: action) } self.timerLock.withLockVoid { assert(self._requestTimer[requestID] == nil) self._requestTimer[requestID] = scheduled } request.req.requestWasQueued(self) }

From the StateMachine:

mutating func timeoutRequest(_ requestID: Request.ID) -> Action { // 1. check requests in queue if let request = self.requests.remove(requestID) { return .init( request: .failRequest(request, HTTPClientError.getConnectionFromPoolTimeout, cancelTimeout: false), connection: .none ) } // 2. This point is reached, because the request may have already been scheduled. A // connection might have become available shortly before the request timeout timer // fired. return .none }

Lukasa · 2021-09-09T14:55:25Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        case .executeRequestsAndCancelTimeouts(let requests, let connection):
+            for request in requests {
+                connection.executeRequest(request.req)
+                self.cancelRequestTimeout(request.id)


Same ordering question here.

For efficiency reasons should we be able to do a "bulk" request timeout cancellation? We have to acquire locks in order to do this (both the timerLock and the event loop locks) so it may be useful to try to coalesce work where we can to try to reduce the pressure on those locks.

Lukasa · 2021-09-09T14:57:40Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+            connectionID: connectionID,
+            http1ConnectionDelegate: self,
+            http2ConnectionDelegate: self,
+            deadline: .now() + (self.clientConfiguration.timeout.connect ?? .seconds(30)),


Can this .seconds(30) be lifted out to a static so that we can see it clearly and give it a name?

glbrntt

This is coming together nicely! It's quite mechanical so it's reasonably easy to review as well.

glbrntt · 2021-09-09T14:09:14Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+    func connectionPoolDidShutdown(_ pool: HTTPConnectionPool, unclean: Bool)
+}
+
+class HTTPConnectionPool {


Suggested change

class HTTPConnectionPool {

final class HTTPConnectionPool {

glbrntt · 2021-09-09T14:09:26Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

@@ -121,6 +128,364 @@ enum HTTPConnectionPool {
            }
        }
    }
+
+    let stateLock = Lock()


Not actioned.

glbrntt · 2021-09-09T14:09:33Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        }
+    }
+
+    let timerLock = Lock()


glbrntt · 2021-09-09T14:46:19Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+
+        case .closeConnection(let connection, isShutdown: let isShutdown):
+            // we are not interested in the close future...
+            _ = connection.close()


Should we add an equivalent close API accepting an optional promise and use that instead?

Great idea. Promise is enough I guess :)

glbrntt · 2021-09-09T14:46:58Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        self.run(action: action)
+    }
+
+    func run(action: StateMachine.Action) {


glbrntt · 2021-09-09T14:50:09Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+    func runRequestAction(_ action: StateMachine.RequestAction) {
+        switch action {
+        case .executeRequest(let request, let connection, cancelTimeout: let cancelTimeout):
+            connection.executeRequest(request.req)


Is the ordering of execute and cancelling the timeout done for any particular reason?

glbrntt · 2021-09-09T14:58:51Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+    // MARK: Run actions
+
+    private func createConnection(_ connectionID: Connection.ID, on eventLoop: EventLoop) {
+        self.connectionFactory.makeConnection(


This API wasn't too clear to me. In my mind "makeThing" should return a "Thing" to the caller. Consider a different name / dropping a comment in here.

glbrntt · 2021-09-09T14:59:58Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+            // The timer has fired. Now we need to do a couple of things:
+            //
+            // 1. Remove ourselves from the timer dictionary to not leak any data. If our
+            //    waiter entry still exist, we need to tell the state machine, that we want


Suggested change

// waiter entry still exist, we need to tell the state machine, that we want

// waiter entry still exists, we need to tell the state machine, that we want

glbrntt · 2021-09-09T15:02:05Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+            //    waiter entry still exist, we need to tell the state machine, that we want
+            //    to fail the request.
+
+            let timeout = self.timerLock.withLock {


nit: timeoutFired / timedOut

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

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

Lukasa · 2021-09-10T07:56:37Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

@@ -121,6 +128,364 @@ enum HTTPConnectionPool {
            }
        }
    }
+
+    let stateLock = Lock()


Not actioned.

Lukasa · 2021-09-10T07:56:50Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        }
+    }
+
+    static let fallbackConnectTimeout: TimeAmount = .seconds(30)


private here too.

Lukasa · 2021-09-10T07:58:39Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+        case .executeRequest(let request, let connection, cancelTimeout: let cancelTimeout):
+            connection.executeRequest(request.req)
+            if cancelTimeout {
+                self.cancelRequestTimeout(request.id)


I think I disagree with your code comment. If we're about to execute a request then we should aim to cancel the timeout first, as this will make some small number of requests succeed where the other order would fail.

glbrntt · 2021-09-10T10:11:14Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+
+    private func createConnection(_ connectionID: Connection.ID, on eventLoop: EventLoop) {
+        // Even though this function is called make it actually creates/establishes a connection.
+        // TBD: Should we rename it? To what?


Maybe just changing "make" to "create" is enough to differentiate make it clear that it's not a factory. Alternatively something like provideNewConnection(to: self, ...)?

fabianfett · 2021-09-10T15:27:35Z

Superseeded by #420

fabianfett force-pushed the ff-connection-pool branch 2 times, most recently from 48dd7a3 to 3c96411 Compare September 8, 2021 22:44

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

fabianfett force-pushed the ff-connection-pool branch from 3c96411 to 7b4da91 Compare September 9, 2021 11:50

fabianfett requested a review from glbrntt September 9, 2021 12:29

fabianfett force-pushed the ff-connection-pool branch 2 times, most recently from 9beffc7 to 4011291 Compare September 9, 2021 13:48

fabianfett requested a review from Lukasa September 9, 2021 13:50

fabianfett force-pushed the ff-connection-pool branch 2 times, most recently from 3aba015 to 5d707f7 Compare September 9, 2021 14:10

Lukasa reviewed Sep 9, 2021

View reviewed changes

glbrntt reviewed Sep 9, 2021

View reviewed changes

Add HTTPConnectionPool

8e4093c

fabianfett force-pushed the ff-connection-pool branch 2 times, most recently from 5cefa51 to 40b6851 Compare September 10, 2021 07:03

fabianfett requested review from Lukasa and glbrntt September 10, 2021 07:03

fabianfett force-pushed the ff-connection-pool branch from 40b6851 to 25d23f4 Compare September 10, 2021 07:13

Code review

ff70dd2

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

fabianfett force-pushed the ff-connection-pool branch from 25d23f4 to ff70dd2 Compare September 10, 2021 07:27

Lukasa reviewed Sep 10, 2021

View reviewed changes

Code review

a3a844e

fabianfett requested a review from Lukasa September 10, 2021 09:06

fabianfett mentioned this pull request Sep 10, 2021

Add new HTTPConnectionPool Manager #420

Merged

glbrntt approved these changes Sep 10, 2021

View reviewed changes

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

fabianfett closed this Sep 10, 2021

fabianfett deleted the ff-connection-pool branch September 14, 2021 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new HTTPConnectionPool #418

Add new HTTPConnectionPool #418

fabianfett commented Sep 8, 2021

Lukasa Sep 9, 2021

fabianfett Sep 9, 2021

Lukasa Sep 10, 2021

fabianfett Sep 10, 2021 •

edited

Loading

Lukasa Sep 9, 2021

Lukasa Sep 9, 2021

Lukasa Sep 9, 2021

glbrntt left a comment

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

Lukasa Sep 10, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

fabianfett Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

Lukasa Sep 10, 2021

Lukasa Sep 10, 2021

Lukasa Sep 10, 2021

glbrntt Sep 10, 2021

fabianfett commented Sep 10, 2021

	// waiter entry still exist, we need to tell the state machine, that we want
	// waiter entry still exists, we need to tell the state machine, that we want

Add new HTTPConnectionPool #418

Add new HTTPConnectionPool #418

Conversation

fabianfett commented Sep 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett Sep 10, 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

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

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

fabianfett commented Sep 10, 2021

fabianfett Sep 10, 2021 •

edited

Loading