From d885a4f5e9e601197490c94dca6e0d99df7fb1e2 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Fri, 1 Jul 2022 16:38:50 -0700 Subject: [PATCH] adopt sendable (#109) motivation: adjust to swift 5.6 changes: * define sendable shims for protocols and structs that may be used in async context * refactor Gauge to include rather than inherit Recorder * adjust tests * add a test to make sure no warning are emitted --- Sources/CoreMetrics/Metrics.swift | 83 +++++++++++++----- Sources/Metrics/Metrics.swift | 12 +-- Sources/MetricsTestKit/TestMetrics.swift | 44 +++++----- Tests/MetricsTests/CoreMetricsTests.swift | 2 +- Tests/MetricsTests/TestMetrics.swift | 27 ++++-- Tests/MetricsTests/TestSendable.swift | 101 ++++++++++++++++++++++ docker/docker-compose.yaml | 2 +- scripts/soundness.sh | 2 +- 8 files changed, 214 insertions(+), 59 deletions(-) create mode 100644 Tests/MetricsTests/TestSendable.swift diff --git a/Sources/CoreMetrics/Metrics.swift b/Sources/CoreMetrics/Metrics.swift index 06fa532..316e73c 100644 --- a/Sources/CoreMetrics/Metrics.swift +++ b/Sources/CoreMetrics/Metrics.swift @@ -12,7 +12,9 @@ // //===----------------------------------------------------------------------===// -// MARK: User API +// MARK: - User API + +// MARK: - Counter extension Counter { /// Create a new `Counter`. @@ -39,9 +41,9 @@ extension Counter { /// This is the user-facing Counter API. /// /// Its behavior depends on the `CounterHandler` implementation. -public class Counter { +public final class Counter { @usableFromInline - var handler: CounterHandler + let handler: CounterHandler public let label: String public let dimensions: [(String, String)] @@ -90,6 +92,8 @@ extension Counter: CustomStringConvertible { } } +// MARK: - FloatingPointCounter + extension FloatingPointCounter { /// Create a new `FloatingPointCounter`. /// @@ -116,9 +120,9 @@ extension FloatingPointCounter { /// This is the user-facing FloatingPointCounter API. /// /// Its behavior depends on the `FloatingCounterHandler` implementation. -public class FloatingPointCounter { +public final class FloatingPointCounter { @usableFromInline - var handler: FloatingPointCounterHandler + let handler: FloatingPointCounterHandler public let label: String public let dimensions: [(String, String)] @@ -167,13 +171,15 @@ extension FloatingPointCounter: CustomStringConvertible { } } -public extension Recorder { +// MARK: - Recorder + +extension Recorder { /// Create a new `Recorder`. /// /// - parameters: /// - label: The label for the `Recorder`. /// - dimensions: The dimensions for the `Recorder`. - convenience init(label: String, dimensions: [(String, String)] = [], aggregate: Bool = true) { + public convenience init(label: String, dimensions: [(String, String)] = [], aggregate: Bool = true) { let handler = MetricsSystem.factory.makeRecorder(label: label, dimensions: dimensions, aggregate: aggregate) self.init(label: label, dimensions: dimensions, aggregate: aggregate, handler: handler) } @@ -181,7 +187,7 @@ public extension Recorder { /// Signal the underlying metrics library that this recorder will never be updated again. /// In response the library MAY decide to eagerly release any resources held by this `Recorder`. @inlinable - func destroy() { + public func destroy() { MetricsSystem.factory.destroyRecorder(self.handler) } } @@ -193,7 +199,7 @@ public extension Recorder { /// Its behavior depends on the `RecorderHandler` implementation. public class Recorder { @usableFromInline - var handler: RecorderHandler + let handler: RecorderHandler public let label: String public let dimensions: [(String, String)] public let aggregate: Bool @@ -247,10 +253,12 @@ extension Recorder: CustomStringConvertible { } } +// MARK: - Gauge + /// A gauge is a metric that represents a single numerical value that can arbitrarily go up and down. /// Gauges are typically used for measured values like temperatures or current memory usage, but also "counts" that can go up and down, like the number of active threads. /// Gauges are modeled as `Recorder` with a sample size of 1 and that does not perform any aggregation. -public class Gauge: Recorder { +public final class Gauge: Recorder { /// Create a new `Gauge`. /// /// - parameters: @@ -261,6 +269,8 @@ public class Gauge: Recorder { } } +// MARK: - Timer + public struct TimeUnit: Equatable { private enum Code: Equatable { case nanoseconds @@ -328,9 +338,9 @@ public extension Timer { /// This is the user-facing Timer API. /// /// Its behavior depends on the `TimerHandler` implementation. -public class Timer { +public final class Timer { @usableFromInline - var handler: TimerHandler + let handler: TimerHandler public let label: String public let dimensions: [(String, String)] @@ -451,6 +461,8 @@ extension Timer: CustomStringConvertible { } } +// MARK: - MetricsSystem + /// The `MetricsSystem` is a global facility where the default metrics backend implementation (`MetricsFactory`) can be /// configured. `MetricsSystem` is set up just once in a given program to set up the desired metrics backend /// implementation. @@ -459,7 +471,7 @@ public enum MetricsSystem { /// `bootstrap` is an one-time configuration function which globally selects the desired metrics backend /// implementation. `bootstrap` can be called at maximum once in any given program, calling it more than once will - /// lead to undefined behaviour, most likely a crash. + /// lead to undefined behavior, most likely a crash. /// /// - parameters: /// - factory: A factory that given an identifier produces instances of metrics handlers such as `CounterHandler`, `RecorderHandler` and `TimerHandler`. @@ -514,7 +526,9 @@ public enum MetricsSystem { } } -// MARK: Library SPI, intended to be implemented by backend libraries +// MARK: - Library SPI, intended to be implemented by backend libraries + +// MARK: - MetricsFactory /// The `MetricsFactory` is the bridge between the `MetricsSystem` and the metrics backend implementation. /// `MetricsFactory`'s role is to initialize concrete implementations of the various metric types: @@ -601,7 +615,7 @@ public protocol MetricsFactory { } /// Wraps a CounterHandler, adding support for incrementing by floating point values by storing an accumulated floating point value and recording increments to the underlying CounterHandler after crossing integer boundaries. -internal class AccumulatingRoundingFloatingPointCounter: FloatingPointCounterHandler { +internal final class AccumulatingRoundingFloatingPointCounter: FloatingPointCounterHandler { private let lock = Lock() private let counterHandler: CounterHandler internal var fraction: Double = 0 @@ -689,6 +703,8 @@ extension MetricsFactory { } } +// MARK: - Backend Handlers + /// A `CounterHandler` represents a backend implementation of a `Counter`. /// /// This type is an implementation detail and should not be used directly, unless implementing your own metrics backend. @@ -700,7 +716,7 @@ extension MetricsFactory { /// as expected regardless of the selected `CounterHandler` implementation. /// /// - The `CounterHandler` must be a `class`. -public protocol CounterHandler: AnyObject { +public protocol CounterHandler: AnyObject, _SwiftMetricsSendableProtocol { /// Increment the counter. /// /// - parameters: @@ -722,7 +738,7 @@ public protocol CounterHandler: AnyObject { /// as expected regardless of the selected `FloatingPointCounterHandler` implementation. /// /// - The `FloatingPointCounterHandler` must be a `class`. -public protocol FloatingPointCounterHandler: AnyObject { +public protocol FloatingPointCounterHandler: AnyObject, _SwiftMetricsSendableProtocol { /// Increment the counter. /// /// - parameters: @@ -744,7 +760,7 @@ public protocol FloatingPointCounterHandler: AnyObject { /// as expected regardless of the selected `RecorderHandler` implementation. /// /// - The `RecorderHandler` must be a `class`. -public protocol RecorderHandler: AnyObject { +public protocol RecorderHandler: AnyObject, _SwiftMetricsSendableProtocol { /// Record a value. /// /// - parameters: @@ -768,7 +784,7 @@ public protocol RecorderHandler: AnyObject { /// as expected regardless of the selected `TimerHandler` implementation. /// /// - The `TimerHandler` must be a `class`. -public protocol TimerHandler: AnyObject { +public protocol TimerHandler: AnyObject, _SwiftMetricsSendableProtocol { /// Record a duration in nanoseconds. /// /// - parameters: @@ -788,7 +804,7 @@ extension TimerHandler { } } -// MARK: Predefined Metrics Handlers +// MARK: - Predefined Metrics Handlers /// A pseudo-metrics handler that can be used to send messages to multiple other metrics handlers. public final class MultiplexMetricsHandler: MetricsFactory { @@ -837,7 +853,7 @@ public final class MultiplexMetricsHandler: MetricsFactory { } } - private class MuxCounter: CounterHandler { + private final class MuxCounter: CounterHandler { let counters: [CounterHandler] public init(factories: [MetricsFactory], label: String, dimensions: [(String, String)]) { self.counters = factories.map { $0.makeCounter(label: label, dimensions: dimensions) } @@ -852,7 +868,7 @@ public final class MultiplexMetricsHandler: MetricsFactory { } } - private class MuxFloatingPointCounter: FloatingPointCounterHandler { + private final class MuxFloatingPointCounter: FloatingPointCounterHandler { let counters: [FloatingPointCounterHandler] public init(factories: [MetricsFactory], label: String, dimensions: [(String, String)]) { self.counters = factories.map { $0.makeFloatingPointCounter(label: label, dimensions: dimensions) } @@ -867,7 +883,7 @@ public final class MultiplexMetricsHandler: MetricsFactory { } } - private class MuxRecorder: RecorderHandler { + private final class MuxRecorder: RecorderHandler { let recorders: [RecorderHandler] public init(factories: [MetricsFactory], label: String, dimensions: [(String, String)], aggregate: Bool) { self.recorders = factories.map { $0.makeRecorder(label: label, dimensions: dimensions, aggregate: aggregate) } @@ -882,7 +898,7 @@ public final class MultiplexMetricsHandler: MetricsFactory { } } - private class MuxTimer: TimerHandler { + private final class MuxTimer: TimerHandler { let timers: [TimerHandler] public init(factories: [MetricsFactory], label: String, dimensions: [(String, String)]) { self.timers = factories.map { $0.makeTimer(label: label, dimensions: dimensions) } @@ -928,3 +944,22 @@ public final class NOOPMetricsHandler: MetricsFactory, CounterHandler, FloatingP public func record(_: Double) {} public func recordNanoseconds(_: Int64) {} } + +// MARK: - Sendable support helpers + +#if compiler(>=5.6) +extension MetricsSystem: Sendable {} +extension Counter: Sendable {} +extension FloatingPointCounter: Sendable {} +// must be @unchecked since Gauge inherits Recorder :( +extension Recorder: @unchecked Sendable {} +extension Timer: Sendable {} +// ideally we would not be using @unchecked here, but concurrency-safety checks do not recognize locks +extension AccumulatingRoundingFloatingPointCounter: @unchecked Sendable {} +#endif + +#if compiler(>=5.6) +@preconcurrency public protocol _SwiftMetricsSendableProtocol: Sendable {} +#else +public protocol _SwiftMetricsSendableProtocol {} +#endif diff --git a/Sources/Metrics/Metrics.swift b/Sources/Metrics/Metrics.swift index 0c8ec89..0d309f9 100644 --- a/Sources/Metrics/Metrics.swift +++ b/Sources/Metrics/Metrics.swift @@ -16,7 +16,7 @@ @_exported import class CoreMetrics.Timer import Foundation -public extension Timer { +extension Timer { /// Convenience for measuring duration of a closure. /// /// - parameters: @@ -24,7 +24,7 @@ public extension Timer { /// - dimensions: The dimensions for the Timer. /// - body: Closure to run & record. @inlinable - static func measure(label: String, dimensions: [(String, String)] = [], body: @escaping () throws -> T) rethrows -> T { + public static func measure(label: String, dimensions: [(String, String)] = [], body: @escaping () throws -> T) rethrows -> T { let timer = Timer(label: label, dimensions: dimensions) let start = DispatchTime.now().uptimeNanoseconds defer { @@ -39,18 +39,18 @@ public extension Timer { /// - parameters: /// - since: Start of the interval as `DispatchTime`. /// - end: End of the interval, defaulting to `.now()`. - func recordInterval(since: DispatchTime, end: DispatchTime = .now()) { + public func recordInterval(since: DispatchTime, end: DispatchTime = .now()) { self.recordNanoseconds(end.uptimeNanoseconds - since.uptimeNanoseconds) } } -public extension Timer { +extension Timer { /// Convenience for recording a duration based on TimeInterval. /// /// - parameters: /// - duration: The duration to record. @inlinable - func record(_ duration: TimeInterval) { + public func record(_ duration: TimeInterval) { self.recordSeconds(duration) } @@ -59,7 +59,7 @@ public extension Timer { /// - parameters: /// - duration: The duration to record. @inlinable - func record(_ duration: DispatchTimeInterval) { + public func record(_ duration: DispatchTimeInterval) { switch duration { case .nanoseconds(let value): self.recordNanoseconds(value) diff --git a/Sources/MetricsTestKit/TestMetrics.swift b/Sources/MetricsTestKit/TestMetrics.swift index bc41fc9..16c8b48 100644 --- a/Sources/MetricsTestKit/TestMetrics.swift +++ b/Sources/MetricsTestKit/TestMetrics.swift @@ -139,14 +139,10 @@ extension TestMetrics.FullKey: Hashable { } } -// ==== ---------------------------------------------------------------------------------------------------------------- - -// MARK: Assertions +// MARK: - Assertions extension TestMetrics { - // ==== ------------------------------------------------------------------------------------------------------------ - - // MARK: Counter + // MARK: - Counter public func expectCounter(_ metric: Counter) throws -> TestCounter { guard let counter = metric.handler as? TestCounter else { @@ -168,9 +164,7 @@ extension TestMetrics { return testCounter } - // ==== ------------------------------------------------------------------------------------------------------------ - - // MARK: Gauge + // MARK: - Gauge public func expectGauge(_ metric: Gauge) throws -> TestRecorder { return try self.expectRecorder(metric) @@ -180,9 +174,7 @@ extension TestMetrics { return try self.expectRecorder(label, dimensions) } - // ==== ------------------------------------------------------------------------------------------------------------ - - // MARK: Recorder + // MARK: - Recorder public func expectRecorder(_ metric: Recorder) throws -> TestRecorder { guard let recorder = metric.handler as? TestRecorder else { @@ -204,9 +196,7 @@ extension TestMetrics { return testRecorder } - // ==== ------------------------------------------------------------------------------------------------------------ - - // MARK: Timer + // MARK: - Timer public func expectTimer(_ metric: CoreMetrics.Timer) throws -> TestTimer { guard let timer = metric.handler as? TestTimer else { @@ -229,9 +219,7 @@ extension TestMetrics { } } -// ==== ---------------------------------------------------------------------------------------------------------------- - -// MARK: Metric type implementations +// MARK: - Metric type implementations public protocol TestMetric { associatedtype Value @@ -418,11 +406,27 @@ extension NSLock { } } -// ==== ---------------------------------------------------------------------------------------------------------------- +// MARK: - Errors -// MARK: Errors +#if compiler(>=5.6) +public enum TestMetricsError: Error { + case missingMetric(label: String, dimensions: [(String, String)]) + case illegalMetricType(metric: Sendable, expected: String) +} +#else public enum TestMetricsError: Error { case missingMetric(label: String, dimensions: [(String, String)]) case illegalMetricType(metric: Any, expected: String) } +#endif + +// MARK: - Sendable support + +#if compiler(>=5.6) +// ideally we would not be using @unchecked here, but concurrency-safety checks do not recognize locks +extension TestMetrics: @unchecked Sendable {} +extension TestCounter: @unchecked Sendable {} +extension TestRecorder: @unchecked Sendable {} +extension TestTimer: @unchecked Sendable {} +#endif diff --git a/Tests/MetricsTests/CoreMetricsTests.swift b/Tests/MetricsTests/CoreMetricsTests.swift index f0c33d2..70affe5 100644 --- a/Tests/MetricsTests/CoreMetricsTests.swift +++ b/Tests/MetricsTests/CoreMetricsTests.swift @@ -384,7 +384,7 @@ class MetricsTests: XCTestCase { } func testCustomFactory() { - class CustomHandler: CounterHandler { + final class CustomHandler: CounterHandler { func increment(by: DataType) where DataType: BinaryInteger {} func reset() {} } diff --git a/Tests/MetricsTests/TestMetrics.swift b/Tests/MetricsTests/TestMetrics.swift index 9796fd7..63f2d35 100644 --- a/Tests/MetricsTests/TestMetrics.swift +++ b/Tests/MetricsTests/TestMetrics.swift @@ -40,8 +40,8 @@ internal final class TestMetrics: MetricsFactory { } private func make(label: String, dimensions: [(String, String)], registry: inout [String: Item], maker: (String, [(String, String)]) -> Item) -> Item { + let item = maker(label, dimensions) return self.lock.withLock { - let item = maker(label, dimensions) registry[label] = item return item } @@ -49,19 +49,25 @@ internal final class TestMetrics: MetricsFactory { func destroyCounter(_ handler: CounterHandler) { if let testCounter = handler as? TestCounter { - self.counters.removeValue(forKey: testCounter.label) + self.lock.withLock { () -> Void in + self.counters.removeValue(forKey: testCounter.label) + } } } func destroyRecorder(_ handler: RecorderHandler) { if let testRecorder = handler as? TestRecorder { - self.recorders.removeValue(forKey: testRecorder.label) + self.lock.withLock { () -> Void in + self.recorders.removeValue(forKey: testRecorder.label) + } } } func destroyTimer(_ handler: TimerHandler) { if let testTimer = handler as? TestTimer { - self.timers.removeValue(forKey: testTimer.label) + self.lock.withLock { () -> Void in + self.timers.removeValue(forKey: testTimer.label) + } } } } @@ -176,8 +182,8 @@ internal class TestTimer: TimerHandler, Equatable { } } -private extension NSLock { - func withLock(_ body: () -> T) -> T { +extension NSLock { + fileprivate func withLock(_ body: () -> T) -> T { self.lock() defer { self.unlock() @@ -185,3 +191,12 @@ private extension NSLock { return body() } } + +// MARK: - Sendable support + +#if compiler(>=5.6) +// ideally we would not be using @unchecked here, but concurrency-safety checks do not recognize locks +extension TestCounter: @unchecked Sendable {} +extension TestRecorder: @unchecked Sendable {} +extension TestTimer: @unchecked Sendable {} +#endif diff --git a/Tests/MetricsTests/TestSendable.swift b/Tests/MetricsTests/TestSendable.swift new file mode 100644 index 0000000..c973aa3 --- /dev/null +++ b/Tests/MetricsTests/TestSendable.swift @@ -0,0 +1,101 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift Metrics API open source project +// +// Copyright (c) 2022 Apple Inc. and the Swift Metrics API project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of Swift Metrics API project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import CoreMetrics +import Dispatch +import XCTest + +class SendableTest: XCTestCase { + #if compiler(>=5.6) + func testSendableMetrics() async { + // bootstrap with our test metrics + let metrics = TestMetrics() + MetricsSystem.bootstrapInternal(metrics) + + do { + let name = "counter-\(NSUUID().uuidString)" + let value = Int.random(in: 0 ... 1000) + let counter = Counter(label: name) + + let task = Task.detached { () -> [Int64] in + counter.increment(by: value) + let handler = counter.handler as! TestCounter + return handler.values.map { $0.1 } + } + let values = await task.value + XCTAssertEqual(values.count, 1, "expected number of entries to match") + XCTAssertEqual(values[0], Int64(value), "expected value to match") + } + + do { + let name = "floating-point-counter-\(NSUUID().uuidString)" + let value = Double.random(in: 0 ... 0.9999) + let counter = FloatingPointCounter(label: name) + + let task = Task.detached { () -> Double in + counter.increment(by: value) + let handler = counter.handler as! AccumulatingRoundingFloatingPointCounter + return handler.fraction + } + let fraction = await task.value + XCTAssertEqual(fraction, value) + } + + do { + let name = "recorder-\(NSUUID().uuidString)" + let value = Double.random(in: -1000 ... 1000) + let recorder = Recorder(label: name) + + let task = Task.detached { () -> [Double] in + recorder.record(value) + let handler = recorder.handler as! TestRecorder + return handler.values.map { $0.1 } + } + let values = await task.value + XCTAssertEqual(values.count, 1, "expected number of entries to match") + XCTAssertEqual(values[0], value, "expected value to match") + } + + do { + let name = "gauge-\(NSUUID().uuidString)" + let value = Double.random(in: -1000 ... 1000) + let gauge = Gauge(label: name) + + let task = Task.detached { () -> [Double] in + gauge.record(value) + let handler = gauge.handler as! TestRecorder + return handler.values.map { $0.1 } + } + let values = await task.value + XCTAssertEqual(values.count, 1, "expected number of entries to match") + XCTAssertEqual(values[0], value, "expected value to match") + } + + do { + let name = "timer-\(NSUUID().uuidString)" + let value = Int64.random(in: 0 ... 1000) + let timer = Timer(label: name) + + let task = Task.detached { () -> [Int64] in + timer.recordNanoseconds(value) + let handler = timer.handler as! TestTimer + return handler.values.map { $0.1 } + } + let values = await task.value + XCTAssertEqual(values.count, 1, "expected number of entries to match") + XCTAssertEqual(values[0], value, "expected value to match") + } + } + #endif +} diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index 7bb11fc..2d817d4 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -40,4 +40,4 @@ services: shell: <<: *common - entrypoint: /bin/bash + entrypoint: /bin/bash -l diff --git a/scripts/soundness.sh b/scripts/soundness.sh index 684c19b..b685f9e 100755 --- a/scripts/soundness.sh +++ b/scripts/soundness.sh @@ -32,7 +32,7 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" function replace_acceptable_years() { # this needs to replace all acceptable forms with 'YEARS' - sed -e 's/2017-2019/YEARS/' -e 's/2018-2019/YEARS/' -e 's/2019/YEARS/' -e 's/2021/YEARS/' + sed -e 's/20[12][789012]-20[12][89012]/YEARS/' -e 's/20[12][89012]/YEARS/' } printf "=> Checking linux tests... "