Skip to content

Commit 90e8b58

Browse files
committed
Avoid precondition failure in write timeout
Motivation: In some cases we can crash because of a precondition failure when the write timeout fires and we aren't in the running state. This can happen for example if the connection is closed whilst the write timer is active. Modifications: Remove the precondition and instead take no action if the timeout fires outside of the running state. Instead we take a new `Action`, `.noAction` when the timer fires. Result: Fewer crashes.
1 parent e69318d commit 90e8b58

File tree

3 files changed

+79
-2
lines changed

3 files changed

+79
-2
lines changed

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
281281
case .close:
282282
context.close(promise: nil)
283283

284-
case .wait:
284+
case .wait, .noAction:
285285
break
286286

287287
case .forwardResponseHead(let head, let pauseRequestBodyStream):

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift

+3-1
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ struct HTTP1ConnectionStateMachine {
8383
case fireChannelActive
8484
case fireChannelInactive
8585
case fireChannelError(Error, closeConnection: Bool)
86+
87+
case noAction
8688
}
8789

8890
private var state: State
@@ -359,7 +361,7 @@ struct HTTP1ConnectionStateMachine {
359361

360362
mutating func idleWriteTimeoutTriggered() -> Action {
361363
guard case .inRequest(var requestStateMachine, let close) = self.state else {
362-
preconditionFailure("Invalid state: \(self.state)")
364+
return .noAction
363365
}
364366

365367
return self.avoidingStateMachineCoW { state -> Action in

Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift

+75
Original file line numberDiff line numberDiff line change
@@ -840,6 +840,81 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
840840
channel.writeAndFlush(request, promise: nil)
841841
XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent])
842842
}
843+
844+
class SlowHandler: ChannelOutboundHandler {
845+
typealias OutboundIn = HTTPClientRequestPart
846+
typealias OutboundOut = HTTPClientRequestPart
847+
848+
func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
849+
context.eventLoop.scheduleTask(in: .milliseconds(300)) {
850+
promise?.succeed()
851+
}
852+
}
853+
}
854+
855+
func testIdleWriteTimeoutOutsideOfRunningState() {
856+
let embedded = EmbeddedChannel()
857+
var maybeTestUtils: HTTP1TestTools?
858+
XCTAssertNoThrow(maybeTestUtils = try embedded.setupHTTP1Connection())
859+
print("pipeline", embedded.pipeline)
860+
guard let testUtils = maybeTestUtils else { return XCTFail("Expected connection setup works") }
861+
862+
var maybeRequest: HTTPClient.Request?
863+
XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "http://localhost/"))
864+
guard var request = maybeRequest else { return XCTFail("Expected to be able to create a request") }
865+
866+
// start a request stream we'll never write to
867+
let streamPromise = embedded.eventLoop.makePromise(of: Void.self)
868+
let streamCallback = { @Sendable (streamWriter: HTTPClient.Body.StreamWriter) -> EventLoopFuture<Void> in
869+
streamPromise.futureResult
870+
}
871+
request.body = .init(contentLength: nil, stream: streamCallback)
872+
873+
let delegate = NullResponseDelegate()
874+
var maybeRequestBag: RequestBag<NullResponseDelegate>?
875+
XCTAssertNoThrow(
876+
maybeRequestBag = try RequestBag(
877+
request: request,
878+
eventLoopPreference: .delegate(on: embedded.eventLoop),
879+
task: .init(eventLoop: embedded.eventLoop, logger: testUtils.logger),
880+
redirectHandler: nil,
881+
connectionDeadline: .now() + .seconds(30),
882+
requestOptions: .forTests(
883+
idleReadTimeout: .milliseconds(10),
884+
idleWriteTimeout: .milliseconds(2)
885+
),
886+
delegate: delegate
887+
)
888+
)
889+
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
890+
891+
testUtils.connection.executeRequest(requestBag)
892+
893+
XCTAssertNoThrow(
894+
try embedded.receiveHeadAndVerify {
895+
XCTAssertEqual($0.method, .GET)
896+
XCTAssertEqual($0.uri, "/")
897+
XCTAssertEqual($0.headers.first(name: "host"), "localhost")
898+
}
899+
)
900+
901+
// close the pipeline to simulate a server-side close
902+
// note this happens before we write so the idle write timeout is still running
903+
try! embedded.pipeline.close().wait()
904+
905+
// advance time to trigger the idle write timeout
906+
// and ensure that the state machine can tolerate this
907+
embedded.embeddedEventLoop.advanceTime(by: .milliseconds(250))
908+
}
909+
}
910+
911+
class NullResponseDelegate: HTTPClientResponseDelegate {
912+
typealias Response = Void
913+
914+
func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task<Void>) throws {
915+
()
916+
}
917+
843918
}
844919

845920
class TestBackpressureWriter {

0 commit comments

Comments
 (0)