Avoid precondition failure in write timeout (#803)

### 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.
* Clear write timeouts upon request completion. When a request completes
we have no use for the idle write timer, we clear the read timer and we
should clear the write one too.

### Result:

Fewer crashes.

The supplied tests fails without these changes and passes with either of them.

Author: rnro

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift

@@ -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:
@@ -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):

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift

@@ -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

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

@@ -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
@@ -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):

Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift

@@ -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 {

Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift

@@ -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)