Always clear read idle timeout at the end of a request

Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ClientChannelHandler.swift

@@ -44,6 +44,7 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
                 }
             } else {
                 self.logger = self.backgroundLogger
+                self.clearIdleReadTimeoutTimer()
                 self.idleReadTimeoutStateMachine = nil
             }
         }
@@ -307,17 +308,18 @@ final class HTTP1ClientChannelHandler: ChannelDuplexHandler {
                 self.run(action, context: context)
             }
 
-        case .clearIdleReadTimeoutTimer:
-            if let oldTimer = self.idleReadTimeoutTimer {
-                self.idleReadTimeoutTimer = nil
-                oldTimer.cancel()
-            }
-
         case .none:
             break
         }
     }
 
+    private func clearIdleReadTimeoutTimer() {
+        if let oldTimer = self.idleReadTimeoutTimer {
+            self.idleReadTimeoutTimer = nil
+            oldTimer.cancel()
+        }
+    }
+
     // MARK: Private HTTPRequestExecutor
 
     private func writeRequestBodyPart0(_ data: IOData, request: HTTPExecutableRequest) {
@@ -415,7 +417,6 @@ struct IdleReadStateMachine {
     enum Action {
         case startIdleReadTimeoutTimer(TimeAmount)
         case resetIdleReadTimeoutTimer(TimeAmount)
-        case clearIdleReadTimeoutTimer
         case none
     }
 
@@ -465,7 +466,7 @@ struct IdleReadStateMachine {
                 return .resetIdleReadTimeoutTimer(self.timeAmount)
             case .end:
                 self.state = .responseEndReceived
-                return .clearIdleReadTimeoutTimer
+                return .none
             }
 
         case .responseEndReceived:

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift

@@ -38,6 +38,7 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
             if let newRequest = self.request, let idleReadTimeout = newRequest.requestOptions.idleReadTimeout {
                 self.idleReadTimeoutStateMachine = .init(timeAmount: idleReadTimeout)
             } else {
+                self.clearIdleReadTimeoutTimer()
                 self.idleReadTimeoutStateMachine = nil
             }
         }
@@ -238,17 +239,18 @@ final class HTTP2ClientRequestHandler: ChannelDuplexHandler {
                 self.run(action, context: context)
             }
 
-        case .clearIdleReadTimeoutTimer:
-            if let oldTimer = self.idleReadTimeoutTimer {
-                self.idleReadTimeoutTimer = nil
-                oldTimer.cancel()
-            }
-
         case .none:
             break
         }
     }
 
+    private func clearIdleReadTimeoutTimer() {
+        if let oldTimer = self.idleReadTimeoutTimer {
+            self.idleReadTimeoutTimer = nil
+            oldTimer.cancel()
+        }
+    }
+
     // MARK: Private HTTPRequestExecutor
 
     private func writeRequestBodyPart0(_ data: IOData, request: HTTPExecutableRequest) {

Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests+XCTest.swift

@@ -29,6 +29,7 @@ extension HTTP1ClientChannelHandlerTests {
             ("testWriteBackpressure", testWriteBackpressure),
             ("testClientHandlerCancelsRequestIfWeWantToShutdown", testClientHandlerCancelsRequestIfWeWantToShutdown),
             ("testIdleReadTimeout", testIdleReadTimeout),
+            ("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled),
             ("testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand", testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand),
         ]
     }

Tests/AsyncHTTPClientTests/HTTP1ClientChannelHandlerTests.swift

@@ -287,6 +287,56 @@ class HTTP1ClientChannelHandlerTests: XCTestCase {
         }
     }
 
+    func testIdleReadTimeoutIsCanceledIfRequestIsCanceled() {
+        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/"))
+        guard let request = maybeRequest else { return XCTFail("Expected to be able to create a request") }
+
+        let delegate = ResponseBackpressureDelegate(eventLoop: embedded.eventLoop)
+        var maybeRequestBag: RequestBag<ResponseBackpressureDelegate>?
+        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(200)),
+            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")
+        })
+        XCTAssertNoThrow(try embedded.receiveEnd())
+
+        let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")]))
+
+        XCTAssertEqual(testUtils.readEventHandler.readHitCounter, 0)
+        embedded.read()
+        XCTAssertEqual(testUtils.readEventHandler.readHitCounter, 1)
+        XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead)))
+
+        // canceling the request
+        requestBag.cancel()
+        XCTAssertThrowsError(try requestBag.task.futureResult.wait()) {
+            XCTAssertEqual($0 as? HTTPClientError, .cancelled)
+        }
+
+        // the idle read timeout should be cleared because we canceled the request
+        // therefore advancing the time should not trigger a crash
+        embedded.embeddedEventLoop.advanceTime(by: .milliseconds(250))
+    }
+
     func testFailHTTPRequestWithContentLengthBecauseOfChannelInactiveWaitingForDemand() {
         let embedded = EmbeddedChannel()
         var maybeTestUtils: HTTP1TestTools?

Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests+XCTest.swift

@@ -28,6 +28,7 @@ extension HTTP2ClientRequestHandlerTests {
             ("testResponseBackpressure", testResponseBackpressure),
             ("testWriteBackpressure", testWriteBackpressure),
             ("testIdleReadTimeout", testIdleReadTimeout),
+            ("testIdleReadTimeoutIsCanceledIfRequestIsCanceled", testIdleReadTimeoutIsCanceledIfRequestIsCanceled),
         ]
     }
 }

Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift

@@ -233,4 +233,56 @@ class HTTP2ClientRequestHandlerTests: XCTestCase {
             XCTAssertEqual($0 as? HTTPClientError, .readTimeout)
         }
     }
+
+    func testIdleReadTimeoutIsCanceledIfRequestIsCanceled() {
+        let embedded = EmbeddedChannel()
+        let readEventHandler = ReadEventHitHandler()
+        let requestHandler = HTTP2ClientRequestHandler(eventLoop: embedded.eventLoop)
+        XCTAssertNoThrow(try embedded.pipeline.syncOperations.addHandlers([readEventHandler, requestHandler]))
+        XCTAssertNoThrow(try embedded.connect(to: .makeAddressResolvingHost("localhost", port: 0)).wait())
+        let logger = Logger(label: "test")
+
+        var maybeRequest: HTTPClient.Request?
+        XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "http://localhost/"))
+        guard let request = maybeRequest else { return XCTFail("Expected to be able to create a request") }
+
+        let delegate = ResponseBackpressureDelegate(eventLoop: embedded.eventLoop)
+        var maybeRequestBag: RequestBag<ResponseBackpressureDelegate>?
+        XCTAssertNoThrow(maybeRequestBag = try RequestBag(
+            request: request,
+            eventLoopPreference: .delegate(on: embedded.eventLoop),
+            task: .init(eventLoop: embedded.eventLoop, logger: logger),
+            redirectHandler: nil,
+            connectionDeadline: .now() + .seconds(30),
+            requestOptions: .forTests(idleReadTimeout: .milliseconds(200)),
+            delegate: delegate
+        ))
+        guard let requestBag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag") }
+
+        embedded.write(requestBag, promise: nil)
+
+        XCTAssertNoThrow(try embedded.receiveHeadAndVerify {
+            XCTAssertEqual($0.method, .GET)
+            XCTAssertEqual($0.uri, "/")
+            XCTAssertEqual($0.headers.first(name: "host"), "localhost")
+        })
+        XCTAssertNoThrow(try embedded.receiveEnd())
+
+        let responseHead = HTTPResponseHead(version: .http1_1, status: .ok, headers: HTTPHeaders([("content-length", "12")]))
+
+        XCTAssertEqual(readEventHandler.readHitCounter, 0)
+        embedded.read()
+        XCTAssertEqual(readEventHandler.readHitCounter, 1)
+        XCTAssertNoThrow(try embedded.writeInbound(HTTPClientResponsePart.head(responseHead)))
+
+        // canceling the request
+        requestBag.cancel()
+        XCTAssertThrowsError(try requestBag.task.futureResult.wait()) {
+            XCTAssertEqual($0 as? HTTPClientError, .cancelled)
+        }
+
+        // the idle read timeout should be cleared because we canceled the request
+        // therefore advancing the time should not trigger a crash
+        embedded.embeddedEventLoop.advanceTime(by: .milliseconds(250))
+    }
 }