fix: Store HTTP telemetry values on Actors to ensure thread safety #807
Conversation
| @@ -221,17 +221,17 @@ public class CRTClientEngine: HTTPClient { | |||
| // END - smithy.client.http.requests.queued_duration | |||
|
|
|||
| // TICK - smithy.client.http.connections.limit | |||
There was a problem hiding this comment.
httpMetricsUsage is now an actor, so changes to it must be made with an async method call, not a synchronous setter.
| @@ -10,7 +10,7 @@ import struct Smithy.AttributeKey | |||
| /// Container for HTTPClient telemetry, including configurable attributes and names. | |||
| /// | |||
| /// Note: This is intended to be used within generated code, not directly. | |||
| public class HttpTelemetry { | |||
| public actor HttpTelemetry { | |||
There was a problem hiding this comment.
HttpTelemetry is now an actor, which enforces exclusive access to its internal data.
| @@ -160,10 +160,30 @@ internal enum HttpMetricsAttributesKeys { | |||
| internal static let serverAddress = AttributeKey<String>(name: "server.address") | |||
| } | |||
|
|
|||
| internal struct HttpMetricsUsage { | |||
| internal actor HttpMetricsUsage { | |||
There was a problem hiding this comment.
HttpMetricsUsage is now an actor, to enforce exclusive access to its properties.
Async setter methods are added for its properties since actor properties may not be set from outside the actor.
| @@ -502,34 +502,37 @@ public final class URLSessionHTTPClient: HTTPClient { | |||
| body = .data(nil) | |||
| } | |||
|
|
|||
| // Capture current time, then spawn a Task to write HTTP telemetry | |||
There was a problem hiding this comment.
Code below is the same as before, except:
- Accesses to
telemetryandtelemetry. httpMetricsUsageare now async since those properties are now actors. - Telemetry code is moved into a spawned Task, which allows for async operations and allows telemetry to be recorded concurrently with continuing the request.
| @@ -72,7 +72,7 @@ extension DefaultTelemetry { | |||
| fileprivate final class NoOpMeter: Meter { | |||
| func createGauge( | |||
| name: String, | |||
| callback: @escaping (any DoubleAsyncMeasurement) -> Void, | |||
| callback: @escaping (any DoubleAsyncMeasurement) async -> Void, | |||
There was a problem hiding this comment.
These callback blocks on DefaultTelemetry are now asynchronous, which allows the block to access properties on actors.
| @@ -18,7 +18,7 @@ public actor SDKLoggingSystem { | |||
| factories[label] = logHandlerFactory | |||
| } | |||
|
|
|||
| public func initialize(defaultLogLevel: SDKLogLevel = .error) async { | |||
| public func initialize(defaultLogLevel: SDKLogLevel = .error) { | |||
There was a problem hiding this comment.
This function has no async operations within, and async is removed. Same for the init below.
| @@ -28,7 +28,7 @@ public protocol Meter: Sendable { | |||
| /// - Returns: handle to stop recording Gauge metrics | |||
| func createGauge( | |||
| name: String, | |||
There was a problem hiding this comment.
Blocks for this protocol are now async to allow for the use of actors.
…mithy-swift into jbe/isolate_http_telemetry
| @@ -502,34 +502,41 @@ public final class URLSessionHTTPClient: HTTPClient { | |||
| body = .data(nil) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Values used by telemetry are captured, then a Task is spawned that does the writing of the telemetry data.
This allows for:
- Request can proceed while telemetry is written
- Task provides async context for access to actors.
|
Closing this PR because the use of Actors to hold telemetry data is incompatible with opentelemetry-swift, which does not generally have asynchronous APIs. Will open a new PR which handles the data race in a way compatible with pre-Swift concurrency code. |
Issue #
awslabs/aws-sdk-swift#1705
Description of changes
HttpTelemetryand associated types by converting them toactor.Metercallbacks have been madeasyncto allow asynchronous calls on actors within.SDKLoggingmethods have been marked non-async.URLSessionHTTPClientis performed on a separate Task that is spawned as a new request is started.Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.