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 2 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 @@ -281,7 +281,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
case .close:
context.close(promise: nil)

case .wait:
case .wait, .noAction:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this new state or can we just re-use .wait?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reuse .wait. .wait is a do nothing. can be renamed in a follow up pr.

break

case .forwardResponseHead(let head, let pauseRequestBodyStream):
Expand Down 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 @@ -83,6 +83,8 @@ struct HTTP1ConnectionStateMachine {
case fireChannelActive
case fireChannelInactive
case fireChannelError(Error, closeConnection: Bool)

case noAction
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove.

}

private var state: State
Expand Down Expand Up @@ -359,7 +361,7 @@ struct HTTP1ConnectionStateMachine {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding another test to HTTP1ConnectionStateMachineTests that just tests this edge?

}

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
75 changes: 75 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,81 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
channel.writeAndFlush(request, promise: nil)
XCTAssertEqual(request.events.map(\.kind), [.willExecuteRequest, .requestHeadSent])
}

class SlowHandler: ChannelOutboundHandler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't used at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, a relic from a previous test approach.

typealias OutboundIn = HTTPClientRequestPart
typealias OutboundOut = HTTPClientRequestPart

func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
context.eventLoop.scheduleTask(in: .milliseconds(300)) {
promise?.succeed()
}
}
}

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 delegate = NullResponseDelegate()
var maybeRequestBag: RequestBag<NullResponseDelegate>?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use a ResponseAccumulator here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't spotted that, thanks.

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: delegate
)
)
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 NullResponseDelegate: HTTPClientResponseDelegate {
typealias Response = Void

func didFinishRequest(task: AsyncHTTPClient.HTTPClient.Task<Void>) throws {
()
}

}

class TestBackpressureWriter {
Expand Down
Loading