Merge pull request #135 from amzn/connection_failures

Identify connection failures so they can be retried uniformly.
amzn · Dec 10, 2023 · 9c71b33 · 9c71b33
2 parents cd6adbf + 677384e
commit 9c71b33
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 29 deletions.
diff --git a/Package.swift b/Package.swift
@@ -42,7 +42,6 @@ let package = Package(
     ],
     dependencies: [
         .package(url: "https://github.com/apple/swift-nio.git", from: "2.33.0"),
-        .package(url: "https://github.com/apple/swift-nio-http2.git", from: "1.19.0"),
         .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.14.0"),
         .package(url: "https://github.com/apple/swift-log.git", from: "1.0.0"),
         .package(url: "https://github.com/apple/swift-metrics.git", "1.0.0"..<"3.0.0"),
@@ -56,7 +55,6 @@ let package = Package(
                 .product(name: "Metrics", package: "swift-metrics"),
                 .product(name: "NIO", package: "swift-nio"),
                 .product(name: "NIOHTTP1", package: "swift-nio"),
-                .product(name: "NIOHTTP2", package: "swift-nio-http2"),
                 .product(name: "NIOFoundationCompat", package: "swift-nio"),
                 .product(name: "NIOSSL", package: "swift-nio-ssl"),
                 .product(name: "AsyncHTTPClient", package: "async-http-client"),

diff --git a/Sources/SmokeHTTPClient/HTTPError.swift b/Sources/SmokeHTTPClient/HTTPError.swift
@@ -34,6 +34,7 @@ public enum HTTPError: Error {
 
     // 5xx
     case connectionError(String)
+    case connectionFailure(cause: Swift.Error)
     case internalServerError(String)
     case invalidRequest(String)
 

diff --git a/Sources/SmokeHTTPClient/HTTPOperationsClient+executeRetriableWithOutput.swift b/Sources/SmokeHTTPClient/HTTPOperationsClient+executeRetriableWithOutput.swift
@@ -21,7 +21,6 @@ import Foundation
 import AsyncHTTPClient
 import NIO
 import NIOHTTP1
-import NIOHTTP2
 import Metrics
 import Tracing
 
@@ -222,9 +221,7 @@ public extension HTTPOperationsClient {
         }
 
         func treatAsAbortedAttempt(cause: Swift.Error) -> Bool {
-            if cause is NIOHTTP2Errors.StreamClosed {
-                return true
-            } else if let clientError = cause as? AsyncHTTPClient.HTTPClientError, clientError == .remoteConnectionClosed {
+            if let httpError = cause as? HTTPError, case .connectionFailure = httpError {
                 return true
             }
 

diff --git a/Sources/SmokeHTTPClient/HTTPOperationsClient+executeRetriableWithoutOutput.swift b/Sources/SmokeHTTPClient/HTTPOperationsClient+executeRetriableWithoutOutput.swift
@@ -21,7 +21,6 @@ import Foundation
 import AsyncHTTPClient
 import NIO
 import NIOHTTP1
-import NIOHTTP2
 import Metrics
 import Tracing
 
@@ -201,9 +200,7 @@ public extension HTTPOperationsClient {
         }
 
         func treatAsAbortedAttempt(cause: Swift.Error) -> Bool {
-            if cause is NIOHTTP2Errors.StreamClosed {
-                return true
-            } else if let clientError = cause as? AsyncHTTPClient.HTTPClientError, clientError == .remoteConnectionClosed {
+            if let httpError = cause as? HTTPError, case .connectionFailure = httpError {
                 return true
             }
 

diff --git a/Sources/SmokeHTTPClient/HTTPOperationsClient.swift b/Sources/SmokeHTTPClient/HTTPOperationsClient.swift
@@ -530,21 +530,19 @@ extension HTTPOperationsClient {
 
         case .failure(let error):
             let wrappingError: HTTPClientError
-
-            let errorDescription = String(describing: error)
-
+
             switch error {
             // for retriable HTTPClientErrors
-            case let clientError as AsyncHTTPClient.HTTPClientError where isRetriableHTTPClientError(clientError: clientError):
-                let cause = HTTPError.connectionError(errorDescription)
-                wrappingError = HTTPClientError(responseCode: 500, cause: cause)
-            // for non-retriable HTTPClientErrors
-            case _ as AsyncHTTPClient.HTTPClientError:
-                let cause = HTTPError.badResponse(errorDescription)
-                wrappingError = HTTPClientError(responseCode: 400, cause: cause)
-            // by default treat all other errors as 500 so they can be retried
+            case let clientError as AsyncHTTPClient.HTTPClientError:
+                if let httpError = asRetriableError(clientError: clientError) {
+                    wrappingError = HTTPClientError(responseCode: 500, cause: httpError)
+                } else {
+                    // for non-retriable HTTPClientErrors
+                    wrappingError = HTTPClientError(responseCode: 400, cause: clientError)
+                }
+            // by default treat all other errors as a 500 connection failure so they can be retried
             default:
-                let cause = HTTPError.connectionError(errorDescription)
+                let cause = HTTPError.connectionFailure(cause: error)
                 wrappingError = HTTPClientError(responseCode: 500, cause: cause)
             }
 
@@ -577,18 +575,20 @@ extension HTTPOperationsClient {
         return providerError
     }
 
-    private func isRetriableHTTPClientError(clientError: AsyncHTTPClient.HTTPClientError) -> Bool {
-        // special case read, connect, connection pool or tls handshake timeouts and remote connection closed errors
-        // to a 500 error to allow for retries
+    private func asRetriableError(clientError: AsyncHTTPClient.HTTPClientError) -> Swift.Error? {
+        // special case read, connect or connection pool errors,
+        // retry these errors according to the retry configuration
         if clientError == AsyncHTTPClient.HTTPClientError.readTimeout
                 || clientError == AsyncHTTPClient.HTTPClientError.connectTimeout
-                || clientError == AsyncHTTPClient.HTTPClientError.tlsHandshakeTimeout
-                || clientError == AsyncHTTPClient.HTTPClientError.remoteConnectionClosed
                 || clientError == AsyncHTTPClient.HTTPClientError.getConnectionFromPoolTimeout {
-            return true
+            return clientError
+        // special case tls handshake timeouts and remote connection closed errors, treat as a connection failures
+        } else if clientError == AsyncHTTPClient.HTTPClientError.tlsHandshakeTimeout
+                || clientError == AsyncHTTPClient.HTTPClientError.remoteConnectionClosed {
+            return HTTPError.connectionFailure(cause: clientError)
         }
 
-        return false
+        return nil
     }
 
     private func getHeadersFromResponse(response: HTTPClient.Response) -> [(String, String)] {