Skip to content

Commit

Permalink
Fix crash when writablity becomes false and races against finishing t…
Browse files Browse the repository at this point in the history
…he http request (#768)

### Motivation

If the channel's writability changed to false just before we finished a
request, we currently run into a precondition.

### Changes

- Remove the precondition and handle the case appropiatly

### Result

A crash less.
  • Loading branch information
fabianfett authored Sep 3, 2024
1 parent 776a1c2 commit 1120541
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,8 @@ struct IdleWriteStateMachine {
self.state = .requestEndSent
return .clearIdleWriteTimeoutTimer
case .waitingForWritabilityEnabled:
preconditionFailure("If the channel is not writable, we can't have sent the request end.")
self.state = .requestEndSent
return .none
case .requestEndSent:
return .none
}
Expand Down
50 changes: 50 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,56 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
}
}

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

var maybeRequest: HTTPClient.Request?
XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "http://localhost/", method: .POST, body: .stream { _ in
// Advance time by more than the idle write timeout (that's 1 millisecond) to trigger the timeout.
let scheduled = embedded.embeddedEventLoop.flatScheduleTask(in: .milliseconds(2)) {
embedded.embeddedEventLoop.makeSucceededVoidFuture()
}
return scheduled.futureResult
}))

guard let request = maybeRequest else { return XCTFail("Expected to be able to create a request") }

let delegate = 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(idleWriteTimeout: .milliseconds(5)),
delegate: delegate
))
guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }

embedded.isWritable = true
embedded.pipeline.fireChannelWritabilityChanged()
testUtils.connection.executeRequest(requestBag)
let expectedHeaders: HTTPHeaders = ["host": "localhost", "Transfer-Encoding": "chunked"]
XCTAssertEqual(
try embedded.readOutbound(as: HTTPClientRequestPart.self),
.head(HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: expectedHeaders))
)

// change the writability to false.
embedded.isWritable = false
embedded.pipeline.fireChannelWritabilityChanged()
embedded.embeddedEventLoop.run()

// let the writer, write an end (while writability is false)
embedded.embeddedEventLoop.advanceTime(by: .milliseconds(2))

XCTAssertEqual(try embedded.readOutbound(as: HTTPClientRequestPart.self), .end(nil))
}

func testIdleWriteTimeoutWritabilityChanged() {
let embedded = EmbeddedChannel()
let testWriter = TestBackpressureWriter(eventLoop: embedded.eventLoop, parts: 5)
Expand Down

0 comments on commit 1120541

Please sign in to comment.