Skip to content

Avoid precondition failure in write timeout #803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
let oldRequest = self.request!
self.request = nil
self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context)
self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context)

switch finalAction {
case .close:
Expand Down Expand Up @@ -353,6 +354,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
let oldRequest = self.request!
self.request = nil
self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context)
self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context)

switch finalAction {
case .close(let writePromise):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ struct HTTP1ConnectionStateMachine {

mutating func idleWriteTimeoutTriggered() -> Action {
guard case .inRequest(var requestStateMachine, let close) = self.state else {
preconditionFailure("Invalid state: \(self.state)")
return .wait
}

return self.avoidingStateMachineCoW { state -> Action in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
self.request!.fail(error)
self.request = nil
self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context)
self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context)
// No matter the error reason, we must always make sure the h2 stream is closed. Only
// once the h2 stream is closed, it is released from the h2 multiplexer. The
// HTTPRequestStateMachine may signal finalAction: .none in the error case (as this is
Expand All @@ -252,6 +253,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
self.request!.succeedRequest(finalParts)
self.request = nil
self.runTimeoutAction(.clearIdleReadTimeoutTimer, context: context)
self.runTimeoutAction(.clearIdleWriteTimeoutTimer, context: context)
self.runSuccessfulFinalAction(finalAction, context: context)

case .failSendBodyPart(let error, let writePromise), .failSendStreamFinished(let error, let writePromise):
Expand Down
55 changes: 55 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,61 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
channel.writeAndFlush(request, promise: nil)
XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent])
}

func testIdleWriteTimeoutOutsideOfRunningState() {
let embedded = EmbeddedChannel()
var maybeTestUtils: HTTP1TestTools?
XCTAssertNoThrow(maybeTestUtils = try embedded.setupHTTP1Connection())
print("pipeline", embedded.pipeline)
guard let testUtils = maybeTestUtils else { return XCTFail("Expected connection setup works") }

var maybeRequest: HTTPClient.Request?
XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "http://localhost/"))
guard var request = maybeRequest else { return XCTFail("Expected to be able to create a request") }

// start a request stream we'll never write to
let streamPromise = embedded.eventLoop.makePromise(of: Void.self)
let streamCallback = { @Sendable (streamWriter: HTTPClient.Body.StreamWriter) -> EventLoopFuture<Void> in
streamPromise.futureResult
}
request.body = .init(contentLength: nil, stream: streamCallback)

let accumulator = ResponseAccumulator(request: request)
var maybeRequestBag: RequestBag<ResponseAccumulator>?
XCTAssertNoThrow(
maybeRequestBag = try RequestBag(
request: request,
eventLoopPreference: .delegate(on: embedded.eventLoop),
task: .init(eventLoop: embedded.eventLoop, logger: testUtils.logger),
redirectHandler: nil,
connectionDeadline: .now() + .seconds(30),
requestOptions: .forTests(
idleReadTimeout: .milliseconds(10),
idleWriteTimeout: .milliseconds(2)
),
delegate: accumulator
)
)
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }

testUtils.connection.executeRequest(requestBag)

XCTAssertNoThrow(
try embedded.receiveHeadAndVerify {
XCTAssertEqual($0.method, .GET)
XCTAssertEqual($0.uri, "/")
XCTAssertEqual($0.headers.first(name: "host"), "localhost")
}
)

// close the pipeline to simulate a server-side close
// note this happens before we write so the idle write timeout is still running
try! embedded.pipeline.close().wait()

// advance time to trigger the idle write timeout
// and ensure that the state machine can tolerate this
embedded.embeddedEventLoop.advanceTime(by: .milliseconds(250))
}
}

class TestBackpressureWriter {
Expand Down
20 changes: 20 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,26 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.read(), .read)
}

func testWriteTimeoutAfterErrorDoesntCrash() {
var state = HTTP1ConnectionStateMachine()
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)

let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/")
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0))
let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata)
XCTAssertEqual(newRequestAction, .sendRequestHead(requestHead, sendEnd: true))
XCTAssertEqual(
state.headSent(),
.notifyRequestHeadSendSuccessfully(resumeRequestBodyStream: false, startIdleTimer: true)
)

struct MyError: Error, Equatable {}
XCTAssertEqual(state.errorHappened(MyError()), .failRequest(MyError(), .close(nil)))

// Primarily we care that we don't crash here
XCTAssertEqual(state.idleWriteTimeoutTriggered(), .wait)
}

func testAConnectionCloseHeaderInTheRequestLeadsToConnectionCloseAfterRequest() {
var state = HTTP1ConnectionStateMachine()
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
Expand Down
Loading