Skip to content

Commit 993fd7c

Browse files
fabianfettfabianfett
committed
Code review part 1
1 parent 1efd68e commit 993fd7c

File tree

4 files changed

+69
-112
lines changed

4 files changed

+69
-112
lines changed

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,7 @@ extension HTTPConnectionPool {
376376
}
377377

378378
mutating func parkConnection(at index: Int) -> (Connection.ID, EventLoop) {
379-
assert(self.connections[index].isIdle)
379+
precondition(self.connections[index].isIdle)
380380
return (self.connections[index].connectionID, self.connections[index].eventLoop)
381381
}
382382

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+32-10
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ extension HTTPConnectionPool {
6363
}
6464
}
6565

66-
mutating func executeRequestOnPreferredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
66+
private mutating func executeRequestOnPreferredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
6767
if let connection = self.connections.leaseConnection(onPreferred: eventLoop) {
6868
return .init(
6969
request: .executeRequest(request, connection, cancelTimeout: nil),
@@ -76,7 +76,7 @@ extension HTTPConnectionPool {
7676
let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
7777
request.connectionDeadline,
7878
for: requestID,
79-
on: request.preferredEventLoop
79+
on: eventLoop
8080
)
8181

8282
if !self.connections.canGrow {
@@ -101,7 +101,7 @@ extension HTTPConnectionPool {
101101
)
102102
}
103103

104-
mutating func executeRequestOnRequiredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
104+
private mutating func executeRequestOnRequiredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
105105
if let connection = self.connections.leaseConnection(onRequired: eventLoop) {
106106
return .init(
107107
request: .executeRequest(request, connection, cancelTimeout: nil),
@@ -114,7 +114,7 @@ extension HTTPConnectionPool {
114114
let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
115115
request.connectionDeadline,
116116
for: requestID,
117-
on: request.preferredEventLoop
117+
on: eventLoop
118118
)
119119

120120
let starting = self.connections.startingEventLoopConnections(on: eventLoop)
@@ -156,12 +156,10 @@ extension HTTPConnectionPool {
156156
// decision about the retry will be made in `connectionCreationBackoffDone(_:)`
157157
let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
158158

159-
let backoff = TimeAmount.milliseconds(100) * Int(pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))
160-
let jitterRange = backoff.nanoseconds / 100 * 5
161-
let jitteredBackoff = backoff + .nanoseconds((-jitterRange...jitterRange).randomElement()!)
159+
let backoff = self.calculateBackoff(for: self.failedConsecutiveConnectionAttempts)
162160
return .init(
163161
request: .none,
164-
connection: .scheduleBackoffTimer(connectionID, backoff: jitteredBackoff, on: eventLoop)
162+
connection: .scheduleBackoffTimer(connectionID, backoff: backoff, on: eventLoop)
165163
)
166164

167165
case .shuttingDown:
@@ -186,7 +184,7 @@ extension HTTPConnectionPool {
186184
return .none
187185
}
188186

189-
assert(self.state == .running, "If we are shutting down, we must not have any idle connections")
187+
precondition(self.state == .running, "If we are shutting down, we must not have any idle connections")
190188

191189
return .init(
192190
request: .none,
@@ -382,7 +380,7 @@ extension HTTPConnectionPool {
382380
return .none
383381

384382
case .shutDown:
385-
preconditionFailure("It the pool is already shutdown, all connections must have been torn down.")
383+
preconditionFailure("If the pool is already shutdown, all connections must have been torn down.")
386384
}
387385
}
388386

@@ -419,6 +417,30 @@ extension HTTPConnectionPool {
419417
self.connections.removeConnection(at: index)
420418
return .none
421419
}
420+
421+
private mutating func calculateBackoff(for attempt: Int) -> TimeAmount {
422+
// Our backoff formula is: 100ms * 1.25^attempt that is capped of at 1minute
423+
// This means for:
424+
// - 1 failed attempt : 100ms
425+
// - 5 failed attempts: ~300ms
426+
// - 10 failed attempts: ~930ms
427+
// - 15 failed attempts: ~2.84s
428+
// - 20 failed attempts: ~8.67s
429+
// - 25 failed attempts: ~26s
430+
// - 29 failed attempts: ~60s (max out)
431+
432+
let start = Double(TimeAmount.milliseconds(100).nanoseconds)
433+
let backoffNanoseconds = Int64(start * pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))
434+
435+
let backoff: TimeAmount = .nanoseconds(backoffNanoseconds)
436+
437+
// Calculate a 3% jitter range
438+
let jitterRange = (backoff.nanoseconds / 100) * 3
439+
// Pick a random element from the range +/- jitter range.
440+
let jitter: TimeAmount = .nanoseconds((-jitterRange...jitterRange).randomElement()!)
441+
let jitteredBackoff = backoff + jitter
442+
return jitteredBackoff
443+
}
422444
}
423445
}
424446

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift

+34-99
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,6 @@ extension HTTPConnectionPool {
6262

6363
enum HTTPTypeStateMachine {
6464
case http1(HTTP1StateMachine)
65-
66-
case _modifying
6765
}
6866

6967
var state: HTTPTypeStateMachine
@@ -85,59 +83,39 @@ extension HTTPConnectionPool {
8583
mutating func executeRequest(_ request: Request) -> Action {
8684
switch self.state {
8785
case .http1(var http1StateMachine):
88-
return self.state.modify { state -> Action in
89-
let action = http1StateMachine.executeRequest(request)
90-
state = .http1(http1StateMachine)
91-
return action
92-
}
93-
94-
case ._modifying:
95-
preconditionFailure("Invalid state: \(self.state)")
86+
let action = http1StateMachine.executeRequest(request)
87+
self.state = .http1(http1StateMachine)
88+
return action
9689
}
9790
}
9891

9992
mutating func newHTTP1ConnectionCreated(_ connection: Connection) -> Action {
10093
switch self.state {
101-
case .http1(var httpStateMachine):
102-
return self.state.modify { state -> Action in
103-
let action = httpStateMachine.newHTTP1ConnectionEstablished(connection)
104-
state = .http1(httpStateMachine)
105-
return action
106-
}
107-
108-
case ._modifying:
109-
preconditionFailure("Invalid state: \(self.state)")
94+
case .http1(var http1StateMachine):
95+
let action = http1StateMachine.newHTTP1ConnectionEstablished(connection)
96+
self.state = .http1(http1StateMachine)
97+
return action
11098
}
11199
}
112100

113101
mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action {
114102
switch self.state {
115103
case .http1(var http1StateMachine):
116-
return self.state.modify { state -> Action in
117-
let action = http1StateMachine.failedToCreateNewConnection(
118-
error,
119-
connectionID: connectionID
120-
)
121-
state = .http1(http1StateMachine)
122-
return action
123-
}
124-
125-
case ._modifying:
126-
preconditionFailure("Invalid state: \(self.state)")
104+
let action = http1StateMachine.failedToCreateNewConnection(
105+
error,
106+
connectionID: connectionID
107+
)
108+
self.state = .http1(http1StateMachine)
109+
return action
127110
}
128111
}
129112

130113
mutating func connectionCreationBackoffDone(_ connectionID: Connection.ID) -> Action {
131114
switch self.state {
132115
case .http1(var http1StateMachine):
133-
return self.state.modify { state -> Action in
134-
let action = http1StateMachine.connectionCreationBackoffDone(connectionID)
135-
state = .http1(http1StateMachine)
136-
return action
137-
}
138-
139-
case ._modifying:
140-
preconditionFailure("Invalid state: \(self.state)")
116+
let action = http1StateMachine.connectionCreationBackoffDone(connectionID)
117+
self.state = .http1(http1StateMachine)
118+
return action
141119
}
142120
}
143121

@@ -149,14 +127,9 @@ extension HTTPConnectionPool {
149127
mutating func timeoutRequest(_ requestID: Request.ID) -> Action {
150128
switch self.state {
151129
case .http1(var http1StateMachine):
152-
return self.state.modify { state -> Action in
153-
let action = http1StateMachine.timeoutRequest(requestID)
154-
state = .http1(http1StateMachine)
155-
return action
156-
}
157-
158-
case ._modifying:
159-
preconditionFailure("Invalid state: \(self.state)")
130+
let action = http1StateMachine.timeoutRequest(requestID)
131+
self.state = .http1(http1StateMachine)
132+
return action
160133
}
161134
}
162135

@@ -168,54 +141,36 @@ extension HTTPConnectionPool {
168141
mutating func cancelRequest(_ requestID: Request.ID) -> Action {
169142
switch self.state {
170143
case .http1(var http1StateMachine):
171-
return self.state.modify { state -> Action in
172-
let action = http1StateMachine.cancelRequest(requestID)
173-
state = .http1(http1StateMachine)
174-
return action
175-
}
176-
177-
case ._modifying:
178-
preconditionFailure("Invalid state: \(self.state)")
144+
let action = http1StateMachine.cancelRequest(requestID)
145+
self.state = .http1(http1StateMachine)
146+
return action
179147
}
180148
}
181149

182150
mutating func connectionIdleTimeout(_ connectionID: Connection.ID) -> Action {
183151
switch self.state {
184152
case .http1(var http1StateMachine):
185-
return self.state.modify { state -> Action in
186-
let action = http1StateMachine.connectionIdleTimeout(connectionID)
187-
state = .http1(http1StateMachine)
188-
return action
189-
}
190-
191-
case ._modifying:
192-
preconditionFailure("Invalid state: \(self.state)")
153+
let action = http1StateMachine.connectionIdleTimeout(connectionID)
154+
self.state = .http1(http1StateMachine)
155+
return action
193156
}
194157
}
195158

196159
/// A connection has been closed
197160
mutating func connectionClosed(_ connectionID: Connection.ID) -> Action {
198161
switch self.state {
199162
case .http1(var http1StateMachine):
200-
return self.state.modify { state -> Action in
201-
let action = http1StateMachine.connectionClosed(connectionID)
202-
state = .http1(http1StateMachine)
203-
return action
204-
}
205-
206-
case ._modifying:
207-
preconditionFailure("Invalid state: \(self.state)")
163+
let action = http1StateMachine.connectionClosed(connectionID)
164+
self.state = .http1(http1StateMachine)
165+
return action
208166
}
209167
}
210168

211169
mutating func http1ConnectionReleased(_ connectionID: Connection.ID) -> Action {
212-
guard case .http1(var http1StateMachine) = self.state else {
213-
preconditionFailure("Invalid state: \(self.state)")
214-
}
215-
216-
return self.state.modify { state -> Action in
170+
switch self.state {
171+
case .http1(var http1StateMachine):
217172
let action = http1StateMachine.http1ConnectionReleased(connectionID)
218-
state = .http1(http1StateMachine)
173+
self.state = .http1(http1StateMachine)
219174
return action
220175
}
221176
}
@@ -229,14 +184,9 @@ extension HTTPConnectionPool {
229184

230185
switch self.state {
231186
case .http1(var http1StateMachine):
232-
return self.state.modify { state -> Action in
233-
let action = http1StateMachine.shutdown()
234-
state = .http1(http1StateMachine)
235-
return action
236-
}
237-
238-
case ._modifying:
239-
preconditionFailure("Invalid state: \(self.state)")
187+
let action = http1StateMachine.shutdown()
188+
self.state = .http1(http1StateMachine)
189+
return action
240190
}
241191
}
242192
}
@@ -262,26 +212,11 @@ extension HTTPConnectionPool {
262212
}
263213
}
264214

265-
extension HTTPConnectionPool.StateMachine.HTTPTypeStateMachine {
266-
mutating func modify<T>(_ closure: (inout Self) throws -> (T)) rethrows -> T {
267-
self = ._modifying
268-
defer {
269-
if case ._modifying = self {
270-
preconditionFailure("Invalid state. Use closure to modify state")
271-
}
272-
}
273-
return try closure(&self)
274-
}
275-
}
276-
277215
extension HTTPConnectionPool.StateMachine: CustomStringConvertible {
278216
var description: String {
279217
switch self.state {
280218
case .http1(let http1):
281219
return ".http1(\(http1))"
282-
283-
case ._modifying:
284-
preconditionFailure("Invalid state: \(self.state)")
285220
}
286221
}
287222
}

Diff for: ‎Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -428,11 +428,11 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
428428
XCTAssertEqual(action.request, .executeRequest(request, connectionToAbort, cancelTimeout: nil))
429429
XCTAssertNoThrow(try connections.execute(mockRequest, on: connectionToAbort))
430430
XCTAssertEqual(connections.parked, 7)
431-
XCTAssertEqual(connections.leased, 1)
431+
XCTAssertEqual(connections.used, 1)
432432
XCTAssertNoThrow(try connections.abortConnection(connectionToAbort.id))
433433
XCTAssertEqual(state.connectionClosed(connectionToAbort.id), .none)
434434
XCTAssertEqual(connections.parked, 7)
435-
XCTAssertEqual(connections.leased, 0)
435+
XCTAssertEqual(connections.used, 0)
436436
}
437437

438438
func testConnectionCloseLeadsToTumbleWeedIfThereNoQueuedRequests() {

0 commit comments

Comments
 (0)