Skip to content

Commit 69f95a5

Browse files
fabianfettfabianfett
committed
code review
1 parent e86dcc9 commit 69f95a5

File tree

4 files changed

+82
-41
lines changed

4 files changed

+82
-41
lines changed

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

+65-34
Original file line numberDiff line numberDiff line change
@@ -122,46 +122,24 @@ final class HTTP2Connection {
122122
return connection.start().map { _ in connection }
123123
}
124124

125-
func execute(request: HTTPExecutableRequest) {
126-
let createStreamChannelPromise = self.channel.eventLoop.makePromise(of: Channel.self)
127-
128-
self.multiplexer.createStreamChannel(promise: createStreamChannelPromise) { channel -> EventLoopFuture<Void> in
129-
do {
130-
// We only support http/2 over an https connection – using the Application-Layer
131-
// Protocol Negotiation (ALPN). For this reason it is safe to fix this to `.https`.
132-
let translate = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https)
133-
let handler = HTTP2ClientRequestHandler(eventLoop: channel.eventLoop)
134-
135-
try channel.pipeline.syncOperations.addHandler(translate)
136-
try channel.pipeline.syncOperations.addHandler(handler)
137-
138-
// We must add the new channel to the list of open channels BEFORE we write the
139-
// request to it. In case of an error, we are sure that the channel was added
140-
// before.
141-
let box = ChannelBox(channel)
142-
self.openStreams.insert(box)
143-
self.channel.closeFuture.whenComplete { _ in
144-
self.openStreams.remove(box)
145-
}
146-
147-
channel.write(request, promise: nil)
148-
return channel.eventLoop.makeSucceededVoidFuture()
149-
} catch {
150-
return channel.eventLoop.makeFailedFuture(error)
125+
func executeRequest(_ request: HTTPExecutableRequest) {
126+
if self.channel.eventLoop.inEventLoop {
127+
self.executeRequest0(request)
128+
} else {
129+
self.channel.eventLoop.execute {
130+
self.executeRequest0(request)
151131
}
152132
}
153-
154-
createStreamChannelPromise.futureResult.whenFailure { error in
155-
request.fail(error)
156-
}
157133
}
158134

159-
func cancel() {
135+
/// shuts down the connection by cancelling all running tasks and closing the connection once
136+
/// all child streams/channels are closed.
137+
func shutdown() {
160138
if self.channel.eventLoop.inEventLoop {
161-
self.cancel0()
139+
self.shutdown0()
162140
} else {
163141
self.channel.eventLoop.execute {
164-
self.cancel0()
142+
self.shutdown0()
165143
}
166144
}
167145
}
@@ -203,7 +181,60 @@ final class HTTP2Connection {
203181
return readyToAcceptConnectionsPromise.futureResult
204182
}
205183

206-
private func cancel0() {
184+
private func executeRequest0(_ request: HTTPExecutableRequest) {
185+
self.channel.eventLoop.assertInEventLoop()
186+
187+
switch self.state {
188+
case .initialized, .starting:
189+
preconditionFailure("Invalid state: \(self.state). Sending requests is not allowed before we are started.")
190+
191+
case .active:
192+
let createStreamChannelPromise = self.channel.eventLoop.makePromise(of: Channel.self)
193+
self.multiplexer.createStreamChannel(promise: createStreamChannelPromise) { channel -> EventLoopFuture<Void> in
194+
do {
195+
// the connection may have been asked to shutdown while we created the child. in
196+
// this
197+
// channel.
198+
guard case .active = self.state else {
199+
throw HTTPClientError.cancelled
200+
}
201+
202+
// We only support http/2 over an https connection – using the Application-Layer
203+
// Protocol Negotiation (ALPN). For this reason it is safe to fix this to `.https`.
204+
let translate = HTTP2FramePayloadToHTTP1ClientCodec(httpProtocol: .https)
205+
let handler = HTTP2ClientRequestHandler(eventLoop: channel.eventLoop)
206+
207+
try channel.pipeline.syncOperations.addHandler(translate)
208+
try channel.pipeline.syncOperations.addHandler(handler)
209+
210+
// We must add the new channel to the list of open channels BEFORE we write the
211+
// request to it. In case of an error, we are sure that the channel was added
212+
// before.
213+
let box = ChannelBox(channel)
214+
self.openStreams.insert(box)
215+
self.channel.closeFuture.whenComplete { _ in
216+
self.openStreams.remove(box)
217+
}
218+
219+
channel.write(request, promise: nil)
220+
return channel.eventLoop.makeSucceededVoidFuture()
221+
} catch {
222+
return channel.eventLoop.makeFailedFuture(error)
223+
}
224+
}
225+
226+
createStreamChannelPromise.futureResult.whenFailure { error in
227+
request.fail(error)
228+
}
229+
230+
case .closing, .closed:
231+
// Because of race conditions requests might reach this point, even though the
232+
// connection is already closing
233+
return request.fail(HTTPClientError.cancelled)
234+
}
235+
}
236+
237+
private func shutdown0() {
207238
self.channel.eventLoop.assertInEventLoop()
208239

209240
self.state = .closing

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

+6-2
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ final class HTTP2IdleHandler<Delegate: HTTP2IdleHandlerDelegate>: ChannelDuplexH
105105
self.run(action, context: context)
106106

107107
default:
108-
context.fireUserInboundEventTriggered(event)
108+
context.triggerUserOutboundEvent(event, promise: promise)
109109
}
110110
}
111111

@@ -240,8 +240,10 @@ extension HTTP2IdleHandler {
240240
return .nothing
241241

242242
case .closing(var openStreams, let maxStreams):
243+
// A stream might be opened, while we are closing because of race conditions. For
244+
// this reason, we should handle this case.
243245
openStreams += 1
244-
self.state = .active(openStreams: openStreams, maxStreams: maxStreams)
246+
self.state = .closing(openStreams: openStreams, maxStreams: maxStreams)
245247
return .nothing
246248

247249
case .initialized, .connected, .closed:
@@ -253,11 +255,13 @@ extension HTTP2IdleHandler {
253255
switch self.state {
254256
case .active(var openStreams, let maxStreams):
255257
openStreams -= 1
258+
assert(openStreams >= 0)
256259
self.state = .active(openStreams: openStreams, maxStreams: maxStreams)
257260
return .notifyConnectionStreamClosed(currentlyAvailable: maxStreams - openStreams)
258261

259262
case .closing(var openStreams, let maxStreams):
260263
openStreams -= 1
264+
assert(openStreams >= 0)
261265
if openStreams == 0 {
262266
self.state = .closed
263267
return .close

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

+8-2
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,10 @@ extension HTTPConnectionPool.ConnectionFactory {
304304
var tlsConfig = self.tlsConfiguration
305305
// since we can support h2, we need to advertise this in alpn
306306
if self.allowHTTP2Connections {
307-
tlsConfig.applicationProtocols = ["http/1.1", "h2"]
307+
// "ProtocolNameList" contains the list of protocols advertised by the
308+
// client, in descending order of preference.
309+
// https://datatracker.ietf.org/doc/html/rfc7301#section-3.1
310+
tlsConfig.applicationProtocols = ["h2", "http/1.1"]
308311
} else {
309312
tlsConfig.applicationProtocols = ["http/1.1"]
310313
}
@@ -406,7 +409,10 @@ extension HTTPConnectionPool.ConnectionFactory {
406409
// since we can support h2, we need to advertise this in alpn
407410
var tlsConfig = self.tlsConfiguration
408411
if self.allowHTTP2Connections {
409-
tlsConfig.applicationProtocols = ["http/1.1", "h2"]
412+
// "ProtocolNameList" contains the list of protocols advertised by the
413+
// client, in descending order of preference.
414+
// https://datatracker.ietf.org/doc/html/rfc7301#section-3.1
415+
tlsConfig.applicationProtocols = ["h2", "http/1.1"]
410416
} else {
411417
tlsConfig.applicationProtocols = ["http/1.1"]
412418
}

Diff for: ‎Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift

+3-3
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ class HTTP2ConnectionTests: XCTestCase {
7777
return XCTFail("Expected to have a request bag at this point")
7878
}
7979

80-
http2Connection.execute(request: requestBag)
80+
http2Connection.executeRequest(requestBag)
8181

8282
XCTAssertEqual(delegate.hitStreamClosed, 0)
8383
var maybeResponse: HTTPClient.Response?
@@ -138,7 +138,7 @@ class HTTP2ConnectionTests: XCTestCase {
138138
return XCTFail("Expected to have a request bag at this point")
139139
}
140140

141-
http2Connection.execute(request: requestBag)
141+
http2Connection.executeRequest(requestBag)
142142

143143
futures.append(requestBag.task.futureResult)
144144
}
@@ -200,7 +200,7 @@ class HTTP2ConnectionTests: XCTestCase {
200200
return XCTFail("Expected to have a request bag at this point")
201201
}
202202

203-
http2Connection.execute(request: requestBag)
203+
http2Connection.executeRequest(requestBag)
204204

205205
XCTAssertEqual(delegate.hitStreamClosed, 0)
206206

0 commit comments

Comments
 (0)