Skip to content

Commit 44efb94

Browse files
fabianfettfabianfett
authored
Cleanup: Connection cancel -> shutdown (#404)
* Cleanup * Code review
1 parent 388b8dc commit 44efb94

10 files changed

+68
-55
lines changed

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ClientChannelHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
163163

164164
func triggerUserOutboundEvent(context: ChannelHandlerContext, event: Any, promise: EventLoopPromise<Void>?) {
165165
switch event {
166-
case HTTPConnectionEvent.cancelRequest:
166+
case HTTPConnectionEvent.shutdownRequested:
167167
self.logger.trace("User outbound event triggered: Cancel request for connection close")
168168
let action = self.state.requestCancelled(closeConnection: true)
169169
self.run(action, context: context)

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1Connection.swift

+3-3
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ final class HTTP1Connection {
6565
return connection
6666
}
6767

68-
func execute(request: HTTPExecutableRequest) {
68+
func executeRequest(_ request: HTTPExecutableRequest) {
6969
if self.channel.eventLoop.inEventLoop {
7070
self.execute0(request: request)
7171
} else {
@@ -75,8 +75,8 @@ final class HTTP1Connection {
7575
}
7676
}
7777

78-
func cancel() {
79-
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.cancelRequest, promise: nil)
78+
func shutdown() {
79+
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
8080
}
8181

8282
func close() -> EventLoopFuture<Void> {

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
126126

127127
func triggerUserOutboundEvent(context: ChannelHandlerContext, event: Any, promise: EventLoopPromise<Void>?) {
128128
switch event {
129-
case HTTPConnectionEvent.cancelRequest:
129+
case HTTPConnectionEvent.shutdownRequested:
130130
let action = self.state.requestCancelled()
131131
self.run(action, context: context)
132132
default:

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -241,12 +241,12 @@ final class HTTP2Connection {
241241

242242
// inform all open streams, that the currently running request should be cancelled.
243243
self.openStreams.forEach { box in
244-
box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.cancelRequest, promise: nil)
244+
box.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
245245
}
246246

247247
// inform the idle connection handler, that connection should be closed, once all streams
248248
// are closed.
249-
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.closeConnection, promise: nil)
249+
self.channel.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
250250
}
251251
}
252252

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ final class HTTP2IdleHandler<Delegate: HTTP2IdleHandlerDelegate>: ChannelDuplexH
100100

101101
func triggerUserOutboundEvent(context: ChannelHandlerContext, event: Any, promise: EventLoopPromise<Void>?) {
102102
switch event {
103-
case HTTPConnectionEvent.closeConnection:
103+
case HTTPConnectionEvent.shutdownRequested:
104104
let action = self.state.closeEventReceived()
105105
self.run(action, context: context)
106106

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionEvent.swift

+1-2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,5 @@
1313
//===----------------------------------------------------------------------===//
1414

1515
enum HTTPConnectionEvent {
16-
case cancelRequest
17-
case closeConnection
16+
case shutdownRequested
1817
}

Diff for: ‎Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift

+49-35
Original file line numberDiff line numberDiff line change
@@ -18,89 +18,103 @@ enum HTTPConnectionPool {
1818
struct Connection: Hashable {
1919
typealias ID = Int
2020

21-
// PLEASE NOTE:
22-
// The HTTP/1.1 connection code here is commented out, for a sad and simple reason: We
23-
// don't have a HTTP1Connection yet. As soon as the HTTP1Connection has landed
24-
// (https://github.com/swift-server/async-http-client/pull/400) we will enable
25-
// HTTP1Connections here. Landing the connection box now enables us to already review the
26-
// ConnectionPool StateMachines.
27-
2821
private enum Reference {
29-
// case http1_1(HTTP1Connection)
30-
22+
case http1_1(HTTP1Connection)
23+
case http2(HTTP2Connection)
3124
case __testOnly_connection(ID, EventLoop)
3225
}
3326

3427
private let _ref: Reference
3528

36-
// fileprivate static func http1_1(_ conn: HTTP1Connection) -> Self {
37-
// Connection(_ref: .http1_1(conn))
38-
// }
29+
fileprivate static func http1_1(_ conn: HTTP1Connection) -> Self {
30+
Connection(_ref: .http1_1(conn))
31+
}
32+
33+
fileprivate static func http2(_ conn: HTTP2Connection) -> Self {
34+
Connection(_ref: .http2(conn))
35+
}
3936

4037
static func __testOnly_connection(id: ID, eventLoop: EventLoop) -> Self {
4138
Connection(_ref: .__testOnly_connection(id, eventLoop))
4239
}
4340

4441
var id: ID {
4542
switch self._ref {
46-
// case .http1_1(let connection):
47-
// return connection.id
43+
case .http1_1(let connection):
44+
return connection.id
45+
case .http2(let connection):
46+
return connection.id
4847
case .__testOnly_connection(let id, _):
4948
return id
5049
}
5150
}
5251

5352
var eventLoop: EventLoop {
5453
switch self._ref {
55-
// case .http1_1(let connection):
56-
// return connection.channel.eventLoop
54+
case .http1_1(let connection):
55+
return connection.channel.eventLoop
56+
case .http2(let connection):
57+
return connection.channel.eventLoop
5758
case .__testOnly_connection(_, let eventLoop):
5859
return eventLoop
5960
}
6061
}
6162

62-
@discardableResult
63-
fileprivate func close() -> EventLoopFuture<Void> {
63+
fileprivate func executeRequest(_ request: HTTPExecutableRequest) {
6464
switch self._ref {
65-
// case .http1_1(let connection):
66-
// return connection.close()
67-
68-
case .__testOnly_connection(_, let eventLoop):
69-
return eventLoop.makeSucceededFuture(())
65+
case .http1_1(let connection):
66+
return connection.executeRequest(request)
67+
case .http2(let connection):
68+
return connection.executeRequest(request)
69+
case .__testOnly_connection:
70+
break
7071
}
7172
}
7273

73-
fileprivate func execute(request: HTTPExecutableRequest) {
74+
/// Shutdown cancels any running requests on the connection and then closes the connection
75+
fileprivate func shutdown() {
7476
switch self._ref {
75-
// case .http1_1(let connection):
76-
// return connection.execute(request: request)
77+
case .http1_1(let connection):
78+
return connection.shutdown()
79+
case .http2(let connection):
80+
return connection.shutdown()
7781
case .__testOnly_connection:
7882
break
7983
}
8084
}
8185

82-
fileprivate func cancel() {
86+
/// Closes the connection without cancelling running requests. Use this when you are sure, that the
87+
/// connection is currently idle.
88+
fileprivate func close() -> EventLoopFuture<Void> {
8389
switch self._ref {
84-
// case .http1_1(let connection):
85-
// return connection.cancel()
86-
case .__testOnly_connection:
87-
break
90+
case .http1_1(let connection):
91+
return connection.close()
92+
case .http2(let connection):
93+
return connection.close()
94+
case .__testOnly_connection(_, let eventLoop):
95+
return eventLoop.makeSucceededFuture(())
8896
}
8997
}
9098

9199
static func == (lhs: HTTPConnectionPool.Connection, rhs: HTTPConnectionPool.Connection) -> Bool {
92100
switch (lhs._ref, rhs._ref) {
93-
// case (.http1_1(let lhsConn), .http1_1(let rhsConn)):
94-
// return lhsConn === rhsConn
101+
case (.http1_1(let lhsConn), .http1_1(let rhsConn)):
102+
return lhsConn.id == rhsConn.id
103+
case (.http2(let lhsConn), .http2(let rhsConn)):
104+
return lhsConn.id == rhsConn.id
95105
case (.__testOnly_connection(let lhsID, let lhsEventLoop), .__testOnly_connection(let rhsID, let rhsEventLoop)):
96106
return lhsID == rhsID && lhsEventLoop === rhsEventLoop
97-
// default:
98-
// return false
107+
default:
108+
return false
99109
}
100110
}
101111

102112
func hash(into hasher: inout Hasher) {
103113
switch self._ref {
114+
case .http1_1(let conn):
115+
hasher.combine(conn.id)
116+
case .http2(let conn):
117+
hasher.combine(conn.id)
104118
case .__testOnly_connection(let id, let eventLoop):
105119
hasher.combine(id)
106120
hasher.combine(eventLoop.id)

Diff for: ‎Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift

+5-5
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
4242
))
4343
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
4444

45-
testUtils.connection.execute(request: requestBag)
45+
testUtils.connection.executeRequest(requestBag)
4646

4747
XCTAssertNoThrow(try embedded.receiveHeadAndVerify {
4848
XCTAssertEqual($0.method, .GET)
@@ -134,7 +134,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
134134
embedded.isWritable = false
135135
testWriter.writabilityChanged(false)
136136
embedded.pipeline.fireChannelWritabilityChanged()
137-
testUtils.connection.execute(request: requestBag)
137+
testUtils.connection.executeRequest(requestBag)
138138

139139
XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .none)
140140

@@ -211,7 +211,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
211211
))
212212
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
213213

214-
testUtils.connection.execute(request: requestBag)
214+
testUtils.connection.executeRequest(requestBag)
215215

216216
XCTAssertNoThrow(try embedded.receiveHeadAndVerify {
217217
XCTAssertEqual($0.method, .GET)
@@ -223,7 +223,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
223223
XCTAssertTrue(embedded.isActive)
224224
XCTAssertEqual(testUtils.connectionDelegate.hitConnectionClosed, 0)
225225
XCTAssertEqual(testUtils.connectionDelegate.hitConnectionReleased, 0)
226-
testUtils.connection.cancel()
226+
testUtils.connection.shutdown()
227227
XCTAssertFalse(embedded.isActive)
228228
embedded.embeddedEventLoop.run()
229229
XCTAssertEqual(testUtils.connectionDelegate.hitConnectionClosed, 1)
@@ -257,7 +257,7 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
257257
))
258258
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
259259

260-
testUtils.connection.execute(request: requestBag)
260+
testUtils.connection.executeRequest(requestBag)
261261

262262
XCTAssertNoThrow(try embedded.receiveHeadAndVerify {
263263
XCTAssertEqual($0.method, .GET)

Diff for: ‎Tests/AsyncHTTPClientTests/HTTP1ConnectionTests.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ class HTTP1ConnectionTests: XCTestCase {
152152
delegate: ResponseAccumulator(request: request)
153153
))
154154
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
155-
connection.execute(request: requestBag)
155+
connection.executeRequest(requestBag)
156156

157157
XCTAssertNoThrow(try server.receiveHeadAndVerify { head in
158158
XCTAssertEqual(head.method, .POST)
@@ -230,7 +230,7 @@ class HTTP1ConnectionTests: XCTestCase {
230230
))
231231
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
232232

233-
connection.execute(request: requestBag)
233+
connection.executeRequest(requestBag)
234234

235235
var response: HTTPClient.Response?
236236
if counter <= closeOnRequest {

Diff for: ‎Tests/AsyncHTTPClientTests/HTTP2IdleHandlerTests.swift

+3-3
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ class HTTP2IdleHandlerTests: XCTestCase {
127127
XCTAssertNoThrow(try embedded.connect(to: .makeAddressResolvingHost("localhost", port: 0)).wait())
128128

129129
XCTAssertTrue(embedded.isActive)
130-
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.closeConnection, promise: nil)
130+
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
131131
XCTAssertFalse(embedded.isActive)
132132
}
133133

@@ -143,7 +143,7 @@ class HTTP2IdleHandlerTests: XCTestCase {
143143
XCTAssertEqual(delegate.maxStreams, 10)
144144

145145
XCTAssertTrue(embedded.isActive)
146-
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.closeConnection, promise: nil)
146+
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
147147
XCTAssertFalse(embedded.isActive)
148148
}
149149

@@ -167,7 +167,7 @@ class HTTP2IdleHandlerTests: XCTestCase {
167167
openStreams.insert(streamID)
168168
}
169169

170-
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.closeConnection, promise: nil)
170+
embedded.pipeline.triggerUserOutboundEvent(HTTPConnectionEvent.shutdownRequested, promise: nil)
171171
XCTAssertTrue(embedded.isActive)
172172

173173
while let streamID = openStreams.randomElement() {

0 commit comments

Comments
 (0)