fix: Fix CRT HTTP client continuation bug #711
Conversation
sichanyoo
changed the title
Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
Outdated
Show resolved
Hide resolved
Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
Outdated
Show resolved
Hide resolved
Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift
Outdated
Show resolved
Hide resolved
| @@ -361,4 +410,36 @@ public class CRTClientEngine: HTTPClient { | |||
| response.body = .stream(stream) | |||
| return requestOptions | |||
| } | |||
|
|
|||
| class ContinuationFlag { | |||
| private var continuationResumed = false | |||
There was a problem hiding this comment.
Rather than have a continuationResumed flag, why not have this class just store the continuation? Once the continuation is resumed, set the reference to it to nil.
That gives you the same indication as the flag, and also protects against calling the continuation twice (be eliminating the reference to it.)
There was a problem hiding this comment.
That's a good point; changed as suggested.
| continuation: continuation, | ||
| http2ManualDataWrites: true | ||
| ) | ||
| Task { |
There was a problem hiding this comment.
What is the purpose of this Task? It contains some code that doesn't appear to be async, then creates yet another async task from inside it.
There was a problem hiding this comment.
I must've forgot to clean it up; deleted as pointed out 👍
| @@ -80,7 +80,7 @@ public extension DefaultSDKRuntimeConfiguration { | |||
| static func makeClient( | |||
| httpClientConfiguration: HttpClientConfiguration = defaultHttpClientConfiguration | |||
| ) -> HTTPClient { | |||
| #if os(iOS) || os(tvOS) || os(watchOS) || os(visionOS) || os(macOS) | |||
| #if os(iOS) || os(tvOS) || os(watchOS) || os(visionOS) | |||
There was a problem hiding this comment.
Let's split this out to a separate PR since it is unrelated to the fix below and these are both significant changes.
sichanyoo
changed the title
…make macOS use URLSession.
| class ContinuationWrapper { | ||
| private var continuation: StreamContinuation? | ||
|
|
||
| public init(_ continuation: StreamContinuation? = nil) { |
There was a problem hiding this comment.
nit: Since you will always create this type with a continuation that is non-nil, make this param required & eliminate the default nil
There was a problem hiding this comment.
Changed as suggested
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Creates a `HTTPRequestOptions` object that can be used to make a HTTP request | ||
| /// - Parameters: | ||
| /// - continuationFlag: The actor that wraps the boolean flag used to track continuation resume status |
There was a problem hiding this comment.
update this doc comment to match current method sig
There was a problem hiding this comment.
Changed as suggested
Issue #
awslabs/aws-sdk-swift#1474
Description of changes
Uses a class that wraps the continuation to ensure the
StreamContinuationthat gets passed around inCRTClientEngine.swiftgets resumed exactly once.*Confirmed with Waqar that the callback functions do not get called concurrently in the underlying aws-crt-swift client engine, hence no need for special handling for the flag to make it concurrency safe.
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.