Skip to content

Commit 14fa6d9

Browse files
dnadobadnadoba
authored
Report last connection error if request deadline is exceeded (#601)
1 parent b3583ba commit 14fa6d9

15 files changed

+205
-51
lines changed

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

-2
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,6 @@ final class HTTPConnectionPool {
147147
self.unlocked = Unlocked(connection: .none, request: .none)
148148

149149
switch stateMachineAction.request {
150-
case .cancelRequestTimeout(let requestID):
151-
self.locked.request = .cancelRequestTimeout(requestID)
152150
case .executeRequest(let request, let connection, cancelTimeout: let cancelTimeout):
153151
if cancelTimeout {
154152
self.locked.request = .cancelRequestTimeout(request.id)

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

+4-2
Original file line numberDiff line numberDiff line change
@@ -323,9 +323,11 @@ extension HTTPConnectionPool {
323323

324324
mutating func cancelRequest(_ requestID: Request.ID) -> Action {
325325
// 1. check requests in queue
326-
if self.requests.remove(requestID) != nil {
326+
if let request = self.requests.remove(requestID) {
327+
// Use the last connection error to let the user know why the request was never scheduled
328+
let error = self.lastConnectFailure ?? HTTPClientError.cancelled
327329
return .init(
328-
request: .cancelRequestTimeout(requestID),
330+
request: .failRequest(request, error, cancelTimeout: true),
329331
connection: .none
330332
)
331333
}

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

+4-2
Original file line numberDiff line numberDiff line change
@@ -444,9 +444,11 @@ extension HTTPConnectionPool {
444444

445445
mutating func cancelRequest(_ requestID: Request.ID) -> Action {
446446
// 1. check requests in queue
447-
if self.requests.remove(requestID) != nil {
447+
if let request = self.requests.remove(requestID) {
448+
// Use the last connection error to let the user know why the request was never scheduled
449+
let error = self.lastConnectFailure ?? HTTPClientError.cancelled
448450
return .init(
449-
request: .cancelRequestTimeout(requestID),
451+
request: .failRequest(request, error, cancelTimeout: true),
450452
connection: .none
451453
)
452454
}

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

-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ extension HTTPConnectionPool {
6161
case failRequestsAndCancelTimeouts([Request], Error)
6262

6363
case scheduleRequestTimeout(for: Request, on: EventLoop)
64-
case cancelRequestTimeout(Request.ID)
6564

6665
case none
6766
}

Diff for: ‎Sources/AsyncHTTPClient/HTTPClient.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -606,7 +606,7 @@ public class HTTPClient {
606606
var deadlineSchedule: Scheduled<Void>?
607607
if let deadline = deadline {
608608
deadlineSchedule = taskEL.scheduleTask(deadline: deadline) {
609-
requestBag.fail(HTTPClientError.deadlineExceeded)
609+
requestBag.deadlineExceeded()
610610
}
611611

612612
task.promise.futureResult.whenComplete { _ in

Diff for: ‎Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

+66-19
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ extension RequestBag {
3131
fileprivate enum State {
3232
case initialized
3333
case queued(HTTPRequestScheduler)
34+
/// if the deadline was exceeded while in the `.queued(_:)` state,
35+
/// we wait until the request pool fails the request with a potential more descriptive error message,
36+
/// if a connection failure has occured while the request was queued.
37+
case deadlineExceededWhileQueued
3438
case executing(HTTPRequestExecutor, RequestStreamState, ResponseStreamState)
3539
case finished(error: Error?)
3640
case redirected(HTTPRequestExecutor, Int, HTTPResponseHead, URL)
@@ -90,13 +94,23 @@ extension RequestBag.StateMachine {
9094
self.state = .queued(scheduler)
9195
}
9296

93-
mutating func willExecuteRequest(_ executor: HTTPRequestExecutor) -> Bool {
97+
enum WillExecuteRequestAction {
98+
case cancelExecuter(HTTPRequestExecutor)
99+
case failTaskAndCancelExecutor(Error, HTTPRequestExecutor)
100+
case none
101+
}
102+
103+
mutating func willExecuteRequest(_ executor: HTTPRequestExecutor) -> WillExecuteRequestAction {
94104
switch self.state {
95105
case .initialized, .queued:
96106
self.state = .executing(executor, .initialized, .initialized)
97-
return true
107+
return .none
108+
case .deadlineExceededWhileQueued:
109+
let error: Error = HTTPClientError.deadlineExceeded
110+
self.state = .finished(error: error)
111+
return .failTaskAndCancelExecutor(error, executor)
98112
case .finished(error: .some):
99-
return false
113+
return .cancelExecuter(executor)
100114
case .executing, .redirected, .finished(error: .none), .modifying:
101115
preconditionFailure("Invalid state: \(self.state)")
102116
}
@@ -110,7 +124,7 @@ extension RequestBag.StateMachine {
110124

111125
mutating func resumeRequestBodyStream() -> ResumeProducingAction {
112126
switch self.state {
113-
case .initialized, .queued:
127+
case .initialized, .queued, .deadlineExceededWhileQueued:
114128
preconditionFailure("A request stream can only be resumed, if the request was started")
115129

116130
case .executing(let executor, .initialized, .initialized):
@@ -150,7 +164,7 @@ extension RequestBag.StateMachine {
150164

151165
mutating func pauseRequestBodyStream() {
152166
switch self.state {
153-
case .initialized, .queued:
167+
case .initialized, .queued, .deadlineExceededWhileQueued:
154168
preconditionFailure("A request stream can only be paused, if the request was started")
155169
case .executing(let executor, let requestState, let responseState):
156170
switch requestState {
@@ -185,7 +199,7 @@ extension RequestBag.StateMachine {
185199

186200
mutating func writeNextRequestPart(_ part: IOData, taskEventLoop: EventLoop) -> WriteAction {
187201
switch self.state {
188-
case .initialized, .queued:
202+
case .initialized, .queued, .deadlineExceededWhileQueued:
189203
preconditionFailure("Invalid state: \(self.state)")
190204
case .executing(let executor, let requestState, let responseState):
191205
switch requestState {
@@ -231,7 +245,7 @@ extension RequestBag.StateMachine {
231245

232246
mutating func finishRequestBodyStream(_ result: Result<Void, Error>) -> FinishAction {
233247
switch self.state {
234-
case .initialized, .queued:
248+
case .initialized, .queued, .deadlineExceededWhileQueued:
235249
preconditionFailure("Invalid state: \(self.state)")
236250
case .executing(let executor, let requestState, let responseState):
237251
switch requestState {
@@ -282,7 +296,7 @@ extension RequestBag.StateMachine {
282296
/// - Returns: Whether the response should be forwarded to the delegate. Will be `false` if the request follows a redirect.
283297
mutating func receiveResponseHead(_ head: HTTPResponseHead) -> ReceiveResponseHeadAction {
284298
switch self.state {
285-
case .initialized, .queued:
299+
case .initialized, .queued, .deadlineExceededWhileQueued:
286300
preconditionFailure("How can we receive a response, if the request hasn't started yet.")
287301
case .executing(let executor, let requestState, let responseState):
288302
guard case .initialized = responseState else {
@@ -328,7 +342,7 @@ extension RequestBag.StateMachine {
328342

329343
mutating func receiveResponseBodyParts(_ buffer: CircularBuffer<ByteBuffer>) -> ReceiveResponseBodyAction {
330344
switch self.state {
331-
case .initialized, .queued:
345+
case .initialized, .queued, .deadlineExceededWhileQueued:
332346
preconditionFailure("How can we receive a response body part, if the request hasn't started yet.")
333347
case .executing(_, _, .initialized):
334348
preconditionFailure("If we receive a response body, we must have received a head before")
@@ -385,7 +399,7 @@ extension RequestBag.StateMachine {
385399

386400
mutating func succeedRequest(_ newChunks: CircularBuffer<ByteBuffer>?) -> ReceiveResponseEndAction {
387401
switch self.state {
388-
case .initialized, .queued:
402+
case .initialized, .queued, .deadlineExceededWhileQueued:
389403
preconditionFailure("How can we receive a response body part, if the request hasn't started yet.")
390404
case .executing(_, _, .initialized):
391405
preconditionFailure("If we receive a response body, we must have received a head before")
@@ -447,7 +461,7 @@ extension RequestBag.StateMachine {
447461

448462
private mutating func failWithConsumptionError(_ error: Error) -> ConsumeAction {
449463
switch self.state {
450-
case .initialized, .queued:
464+
case .initialized, .queued, .deadlineExceededWhileQueued:
451465
preconditionFailure("Invalid state: \(self.state)")
452466
case .executing(_, _, .initialized):
453467
preconditionFailure("Invalid state: Must have received response head, before this method is called for the first time")
@@ -482,7 +496,7 @@ extension RequestBag.StateMachine {
482496

483497
private mutating func consumeMoreBodyData() -> ConsumeAction {
484498
switch self.state {
485-
case .initialized, .queued:
499+
case .initialized, .queued, .deadlineExceededWhileQueued:
486500
preconditionFailure("Invalid state: \(self.state)")
487501

488502
case .executing(_, _, .initialized):
@@ -532,8 +546,33 @@ extension RequestBag.StateMachine {
532546
}
533547
}
534548

549+
enum DeadlineExceededAction {
550+
case cancelScheduler(HTTPRequestScheduler?)
551+
case fail(FailAction)
552+
}
553+
554+
mutating func deadlineExceeded() -> DeadlineExceededAction {
555+
switch self.state {
556+
case .queued(let queuer):
557+
/// We do not fail the request immediately because we want to give the scheduler a chance of throwing a better error message
558+
/// We therefore depend on the scheduler failing the request after we cancel the request.
559+
self.state = .deadlineExceededWhileQueued
560+
return .cancelScheduler(queuer)
561+
562+
case .initialized,
563+
.deadlineExceededWhileQueued,
564+
.executing,
565+
.finished,
566+
.redirected,
567+
.modifying:
568+
/// if we are not in the queued state, we can fail early by just calling down to `self.fail(_:)`
569+
/// which does the appropriate state transition for us.
570+
return .fail(self.fail(HTTPClientError.deadlineExceeded))
571+
}
572+
}
573+
535574
enum FailAction {
536-
case failTask(HTTPRequestScheduler?, HTTPRequestExecutor?)
575+
case failTask(Error, HTTPRequestScheduler?, HTTPRequestExecutor?)
537576
case cancelExecutor(HTTPRequestExecutor)
538577
case none
539578
}
@@ -542,31 +581,39 @@ extension RequestBag.StateMachine {
542581
switch self.state {
543582
case .initialized:
544583
self.state = .finished(error: error)
545-
return .failTask(nil, nil)
584+
return .failTask(error, nil, nil)
546585
case .queued(let queuer):
547586
self.state = .finished(error: error)
548-
return .failTask(queuer, nil)
587+
return .failTask(error, queuer, nil)
549588
case .executing(let executor, let requestState, .buffering(_, next: .eof)):
550589
self.state = .executing(executor, requestState, .buffering(.init(), next: .error(error)))
551590
return .cancelExecutor(executor)
552591
case .executing(let executor, _, .buffering(_, next: .askExecutorForMore)):
553592
self.state = .finished(error: error)
554-
return .failTask(nil, executor)
593+
return .failTask(error, nil, executor)
555594
case .executing(let executor, _, .buffering(_, next: .error(_))):
556595
// this would override another error, let's keep the first one
557596
return .cancelExecutor(executor)
558597
case .executing(let executor, _, .initialized):
559598
self.state = .finished(error: error)
560-
return .failTask(nil, executor)
599+
return .failTask(error, nil, executor)
561600
case .executing(let executor, _, .waitingForRemote):
562601
self.state = .finished(error: error)
563-
return .failTask(nil, executor)
602+
return .failTask(error, nil, executor)
564603
case .redirected:
565604
self.state = .finished(error: error)
566-
return .failTask(nil, nil)
605+
return .failTask(error, nil, nil)
567606
case .finished(.none):
568607
// An error occurred after the request has finished. Ignore...
569608
return .none
609+
case .deadlineExceededWhileQueued:
610+
// if we just get a `HTTPClientError.cancelled` we can use the original cancellation reason
611+
// to give a more descriptive error to the user.
612+
if (error as? HTTPClientError) == .cancelled {
613+
return .failTask(HTTPClientError.deadlineExceeded, nil, nil)
614+
}
615+
// otherwise we already had an intermediate connection error which we should present to the user instead
616+
return .failTask(error, nil, nil)
570617
case .finished(.some(_)):
571618
// this might happen, if the stream consumer has failed... let's just drop the data
572619
return .none

Diff for: ‎Sources/AsyncHTTPClient/RequestBag.swift

+38-10
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,16 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
8181

8282
private func willExecuteRequest0(_ executor: HTTPRequestExecutor) {
8383
self.task.eventLoop.assertInEventLoop()
84-
if !self.state.willExecuteRequest(executor) {
85-
return executor.cancelRequest(self)
84+
let action = self.state.willExecuteRequest(executor)
85+
switch action {
86+
case .cancelExecuter(let executor):
87+
executor.cancelRequest(self)
88+
case .failTaskAndCancelExecutor(let error, let executor):
89+
self.delegate.didReceiveError(task: self.task, error)
90+
self.task.fail(with: error, delegateType: Delegate.self)
91+
executor.cancelRequest(self)
92+
case .none:
93+
break
8694
}
8795
}
8896

@@ -320,8 +328,12 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
320328

321329
let action = self.state.fail(error)
322330

331+
self.executeFailAction0(action)
332+
}
333+
334+
private func executeFailAction0(_ action: RequestBag<Delegate>.StateMachine.FailAction) {
323335
switch action {
324-
case .failTask(let scheduler, let executor):
336+
case .failTask(let error, let scheduler, let executor):
325337
scheduler?.cancelRequest(self)
326338
executor?.cancelRequest(self)
327339
self.failTask0(error)
@@ -331,6 +343,28 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
331343
break
332344
}
333345
}
346+
347+
func deadlineExceeded0() {
348+
self.task.eventLoop.assertInEventLoop()
349+
let action = self.state.deadlineExceeded()
350+
351+
switch action {
352+
case .cancelScheduler(let scheduler):
353+
scheduler?.cancelRequest(self)
354+
case .fail(let failAction):
355+
self.executeFailAction0(failAction)
356+
}
357+
}
358+
359+
func deadlineExceeded() {
360+
if self.task.eventLoop.inEventLoop {
361+
self.deadlineExceeded0()
362+
} else {
363+
self.task.eventLoop.execute {
364+
self.deadlineExceeded0()
365+
}
366+
}
367+
}
334368
}
335369

336370
extension RequestBag: HTTPSchedulableRequest {
@@ -457,12 +491,6 @@ extension RequestBag: HTTPExecutableRequest {
457491

458492
extension RequestBag: HTTPClientTaskDelegate {
459493
func cancel() {
460-
if self.task.eventLoop.inEventLoop {
461-
self.fail0(HTTPClientError.cancelled)
462-
} else {
463-
self.task.eventLoop.execute {
464-
self.fail0(HTTPClientError.cancelled)
465-
}
466-
}
494+
self.fail(HTTPClientError.cancelled)
467495
}
468496
}

Diff for: ‎Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift

+1
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ extension HTTPClientTests {
7979
("testStressGetHttps", testStressGetHttps),
8080
("testStressGetHttpsSSLError", testStressGetHttpsSSLError),
8181
("testSelfSignedCertificateIsRejectedWithCorrectError", testSelfSignedCertificateIsRejectedWithCorrectError),
82+
("testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded", testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded),
8283
("testFailingConnectionIsReleased", testFailingConnectionIsReleased),
8384
("testResponseDelayGet", testResponseDelayGet),
8485
("testIdleTimeoutNoReuse", testIdleTimeoutNoReuse),

Diff for: ‎Tests/AsyncHTTPClientTests/HTTPClientTests.swift

+41
Original file line numberDiff line numberDiff line change
@@ -1269,6 +1269,47 @@ class HTTPClientTests: XCTestCase {
12691269
}
12701270
}
12711271

1272+
func testSelfSignedCertificateIsRejectedWithCorrectErrorIfRequestDeadlineIsExceeded() throws {
1273+
/// key + cert was created with the follwing command:
1274+
/// openssl req -x509 -newkey rsa:4096 -keyout self_signed_key.pem -out self_signed_cert.pem -sha256 -days 99999 -nodes -subj '/CN=localhost'
1275+
let certPath = Bundle.module.path(forResource: "self_signed_cert", ofType: "pem")!
1276+
let keyPath = Bundle.module.path(forResource: "self_signed_key", ofType: "pem")!
1277+
let configuration = TLSConfiguration.makeServerConfiguration(
1278+
certificateChain: try NIOSSLCertificate.fromPEMFile(certPath).map { .certificate($0) },
1279+
privateKey: .file(keyPath)
1280+
)
1281+
let sslContext = try NIOSSLContext(configuration: configuration)
1282+
1283+
let server = ServerBootstrap(group: serverGroup)
1284+
.childChannelInitializer { channel in
1285+
channel.pipeline.addHandler(NIOSSLServerHandler(context: sslContext))
1286+
}
1287+
let serverChannel = try server.bind(host: "localhost", port: 0).wait()
1288+
defer { XCTAssertNoThrow(try serverChannel.close().wait()) }
1289+
let port = serverChannel.localAddress!.port!
1290+
1291+
var config = HTTPClient.Configuration()
1292+
config.timeout.connect = .seconds(3)
1293+
let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup), configuration: config)
1294+
defer { XCTAssertNoThrow(try localClient.syncShutdown()) }
1295+
1296+
XCTAssertThrowsError(try localClient.get(url: "https://localhost:\(port)", deadline: .now() + .seconds(2)).wait()) { error in
1297+
#if canImport(Network)
1298+
guard let nwTLSError = error as? HTTPClient.NWTLSError else {
1299+
XCTFail("could not cast \(error) of type \(type(of: error)) to \(HTTPClient.NWTLSError.self)")
1300+
return
1301+
}
1302+
XCTAssertEqual(nwTLSError.status, errSSLBadCert, "unexpected tls error: \(nwTLSError)")
1303+
#else
1304+
guard let sslError = error as? NIOSSLError,
1305+
case .handshakeFailed(.sslError) = sslError else {
1306+
XCTFail("unexpected error \(error)")
1307+
return
1308+
}
1309+
#endif
1310+
}
1311+
}
1312+
12721313
func testFailingConnectionIsReleased() {
12731314
let localHTTPBin = HTTPBin(.refuse)
12741315
let localClient = HTTPClient(eventLoopGroupProvider: .shared(self.clientGroup))

0 commit comments

Comments
 (0)