feat: Add socketTimeout to HTTP client config for closing stale connections #683
Conversation
…stream in CRTClientEngine in HTTPMonitoringOptions constructor. Update comments to clarify the preexisting connetTimeout config's usage. Fix URLSession client to use the new socketTimeout config for timeoutIntervalForRequest instead of connectTimeout.
…alizers. Instead, use 0 (no socket timeout) if it was not provided by end-user in the config.
| @@ -338,6 +346,7 @@ public class CRTClientEngine: HTTPClient { | |||
| response.statusCode = makeStatusCode(statusCode) | |||
| case .failure(let error): | |||
| self.logger.error("Response encountered an error: \(error)") | |||
| continuation.resume(throwing: error) | |||
| /// Used to close stale connections that have no activity. | ||
| /// | ||
| /// If no value is provided, the defaut client won't have a socket timeout. | ||
| public var socketTimeout: TimeInterval? |
There was a problem hiding this comment.
URLSession does not have separate settings for timeout on initial connection, and for idle time between data transmissions on an existing connection.
seems to be the setting for both and the two cannot be set individually.
Our doc comments should make clear which settings apply to which HTTP client and which are ignored.
There was a problem hiding this comment.
Found a separate setting for initial connection timeout in URLRequest: https://developer.apple.com/documentation/foundation/urlrequest/2011509-timeoutinterval/
connectTimeout config on HttpClientConiguration is now set for URLRequest:timeoutInterval.
| if let connectTimeout = httpClientConfiguration.connectTimeout { | ||
| config.timeoutIntervalForRequest = connectTimeout | ||
| if let socketTimeout = httpClientConfiguration.socketTimeout { | ||
| config.timeoutIntervalForRequest = socketTimeout |
There was a problem hiding this comment.
See my note above. How we transfer the HTTP config fields to specific clients should be made clear in the doc comments.
There was a problem hiding this comment.
Both config fields are now used for same purposes in both clients, so I think existing comments do their job without exposing arbitrary details. Lmk if you think otherwise, as I may be missing something.
…ctions (#683) * Add socketTimeout to HttpClientConfiguration, and use that value downstream in CRTClientEngine in HTTPMonitoringOptions constructor. Update comments to clarify the preexisting connetTimeout config's usage. Fix URLSession client to use the new socketTimeout config for timeoutIntervalForRequest instead of connectTimeout. * Bump CRT version to 0.29.0. * Update comments & delete default value of 2 seconds from config initializers. Instead, use 0 (no socket timeout) if it was not provided by end-user in the config. * Set connection timeout for URLSessionHTTPClient as well, using URLRequest:timeoutInterval config. --------- Co-authored-by: Sichan Yoo <chanyoo@amazon.com>
Issue #
awslabs/aws-sdk-swift#1349
Companion PR: awslabs/aws-sdk-swift#1413
Description of changes
socketTimeout, which gets used to constructHTTPMonitoringOptionsforCRTClientEngine. Sets the maximum time to wait between data packets, which is used to close stale connections.URLSessionHTTPClientto usesocketTimeoutfor itstimeoutIntervalForRequestconfig instead ofconnectTimeout. Refer to this official doc form Apple, which confirms thattimeoutIntervalForRequestmaps to the newsocketTimeoutrather thanconnectTimeout, which is a connection timeout config.continuation.resume()that makes the application hang when socket timeout error is thrown.connectTimeoutactually is for. It's not a socket timeout for limiting stale connections. It's used to limit time it takes to obtain a live connection to the server.Prerequisite
HTTPMonitoringOptionspublic for usage in smithy-swift.Testing
Tested against localhost that mocks DynamoDB & does not return a response. HTTP client connection now closes and recovers from no server response as it should, instead of hanging for eternity.
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.