From aabe8eb57f364574c19018ee8ca183a27f323761 Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Fri, 11 Aug 2023 17:36:23 +0200 Subject: [PATCH 1/3] Enable strict concurrency checking in CI --- Package.swift | 4 +- .../InstrumentationSystem.swift | 74 ++++++++++++------- Sources/Instrumentation/Locks.swift | 26 ++++++- Sources/Tracing/NoOpTracer.swift | 17 +++-- Sources/Tracing/SpanProtocol.swift | 24 +++--- Sources/Tracing/Tracer.swift | 51 ++++++++++++- .../_TracingBenchmarkTools/ArgParser.swift | 14 +++- .../BenchmarkCategory.swift | 2 +- .../BenchmarkTools.swift | 35 ++++++--- .../_TracingBenchmarkTools/DriverUtils.swift | 18 +++-- .../SpanAttributesDSLBenchmark.swift | 49 +++++++----- Sources/_TracingBenchmarks/main.swift | 4 +- Tests/TracingTests/ActorTracingTests.swift | 14 ++-- .../DynamicTracepointTracerTests.swift | 34 +++++---- Tests/TracingTests/TracedLock.swift | 10 ++- Tests/TracingTests/TracerTests.swift | 53 ++++++------- docker/docker-compose.2004.58.yaml | 1 + docker/docker-compose.2204.59.yaml | 1 + docker/docker-compose.2204.main.yaml | 1 + docker/docker-compose.yaml | 2 +- 20 files changed, 293 insertions(+), 141 deletions(-) diff --git a/Package.swift b/Package.swift index f2a6c5c9..c7e67764 100644 --- a/Package.swift +++ b/Package.swift @@ -57,7 +57,9 @@ let package = Package( ), .target( name: "_TracingBenchmarkTools", - dependencies: [], + dependencies: [ + .target(name: "Instrumentation"), + ], exclude: ["README_SWIFT.md"] ), ] diff --git a/Sources/Instrumentation/InstrumentationSystem.swift b/Sources/Instrumentation/InstrumentationSystem.swift index acb9eaa0..ae4da76b 100644 --- a/Sources/Instrumentation/InstrumentationSystem.swift +++ b/Sources/Instrumentation/InstrumentationSystem.swift @@ -24,41 +24,71 @@ import ServiceContextModule /// ``instrument``: Returns whatever you passed to ``bootstrap(_:)`` as an ``Instrument``. @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext public enum InstrumentationSystem { - private static let lock = ReadWriteLock() - private static var _instrument: Instrument = NoOpInstrument() - private static var isInitialized = false + /// Marked as @unchecked Sendable due to the synchronization being + /// performed manually using locks. + private final class Storage: @unchecked Sendable { + private let lock = ReadWriteLock() + private var _instrument: Instrument = NoOpInstrument() + private var _isInitialized = false + + func bootstrap(_ instrument: Instrument) { + self.lock.withWriterLock { + precondition( + !self._isInitialized, """ + InstrumentationSystem can only be initialized once per process. Consider using MultiplexInstrument if + you need to use multiple instruments. + """ + ) + self._instrument = instrument + self._isInitialized = true + } + } + + func bootstrapInternal(_ instrument: Instrument?) { + self.lock.withWriterLock { + self._instrument = instrument ?? NoOpInstrument() + } + } + + var instrument: Instrument { + self.lock.withReaderLock { self._instrument } + } + + func _findInstrument(where predicate: (Instrument) -> Bool) -> Instrument? { + self.lock.withReaderLock { + if let multiplex = self._instrument as? MultiplexInstrument { + return multiplex.firstInstrument(where: predicate) + } else if predicate(self._instrument) { + return self._instrument + } else { + return nil + } + } + } + } + + private static let shared = Storage() /// Globally select the desired ``Instrument`` implementation. /// /// - Parameter instrument: The ``Instrument`` you want to share globally within your system. /// - Warning: Do not call this method more than once. This will lead to a crash. public static func bootstrap(_ instrument: Instrument) { - self.lock.withWriterLock { - precondition( - !self.isInitialized, """ - InstrumentationSystem can only be initialized once per process. Consider using MultiplexInstrument if - you need to use multiple instruments. - """ - ) - self._instrument = instrument - self.isInitialized = true - } + self.shared.bootstrap(instrument) } /// For testing scenarios one may want to set instruments multiple times, rather than the set-once semantics enforced by ``bootstrap(_:)``. /// /// - Parameter instrument: the instrument to boostrap the system with, if `nil` the ``NoOpInstrument`` is bootstrapped. internal static func bootstrapInternal(_ instrument: Instrument?) { - self.lock.withWriterLock { - self._instrument = instrument ?? NoOpInstrument() - } + self.shared.bootstrapInternal(instrument) } /// Returns the globally configured ``Instrument``. /// /// Defaults to a no-op ``Instrument`` if ``bootstrap(_:)`` wasn't called before. public static var instrument: Instrument { - self.lock.withReaderLock { self._instrument } + shared.instrument } } @@ -66,14 +96,6 @@ public enum InstrumentationSystem { extension InstrumentationSystem { /// INTERNAL API: Do Not Use public static func _findInstrument(where predicate: (Instrument) -> Bool) -> Instrument? { - self.lock.withReaderLock { - if let multiplex = self._instrument as? MultiplexInstrument { - return multiplex.firstInstrument(where: predicate) - } else if predicate(self._instrument) { - return self._instrument - } else { - return nil - } - } + self.shared._findInstrument(where: predicate) } } diff --git a/Sources/Instrumentation/Locks.swift b/Sources/Instrumentation/Locks.swift index 606432f7..54f1d95c 100644 --- a/Sources/Instrumentation/Locks.swift +++ b/Sources/Instrumentation/Locks.swift @@ -37,7 +37,8 @@ import Glibc /// This object provides a lock on top of a single `pthread_mutex_t`. This kind /// of lock is safe to use with `libpthread`-based threading models, such as the /// one used by NIO. -internal final class ReadWriteLock { +@_spi(Locking) /* Use the `package` access modifier once min Swift version is increased. */ +public final class ReadWriteLock { private let rwlock: UnsafeMutablePointer = UnsafeMutablePointer.allocate(capacity: 1) /// Create a new lock. @@ -90,7 +91,7 @@ extension ReadWriteLock { /// - Parameter body: The block to execute while holding the lock. /// - Returns: The value returned by the block. @inlinable - func withReaderLock(_ body: () throws -> T) rethrows -> T { + public func withReaderLock(_ body: () throws -> T) rethrows -> T { self.lockRead() defer { self.unlock() @@ -107,7 +108,7 @@ extension ReadWriteLock { /// - Parameter body: The block to execute while holding the lock. /// - Returns: The value returned by the block. @inlinable - func withWriterLock(_ body: () throws -> T) rethrows -> T { + public func withWriterLock(_ body: () throws -> T) rethrows -> T { self.lockWrite() defer { self.unlock() @@ -115,3 +116,22 @@ extension ReadWriteLock { return try body() } } + +/// A wrapper providing locked access to a value. +/// +/// Marked as @unchecked Sendable due to the synchronization being +/// performed manually using locks. +@_spi(Locking) /* Use the `package` access modifier once min Swift version is increased. */ +public final class LockedValueBox: @unchecked Sendable { + private let lock = ReadWriteLock() + private var value: Value + public init(_ value: Value) { + self.value = value + } + + public func withValue(_ work: (inout Value) throws -> R) rethrows -> R { + try self.lock.withWriterLock { + try work(&self.value) + } + } +} diff --git a/Sources/Tracing/NoOpTracer.swift b/Sources/Tracing/NoOpTracer.swift index b0d7f72b..8f1fc9de 100644 --- a/Sources/Tracing/NoOpTracer.swift +++ b/Sources/Tracing/NoOpTracer.swift @@ -23,14 +23,15 @@ public struct NoOpTracer: LegacyTracer { public init() {} - public func startAnySpan(_ operationName: String, - context: @autoclosure () -> ServiceContext, - ofKind kind: SpanKind, - at instant: @autoclosure () -> Instant, - function: String, - file fileID: String, - line: UInt) -> any Tracing.Span - { + public func startAnySpan( + _ operationName: String, + context: @autoclosure () -> ServiceContext, + ofKind kind: SpanKind, + at instant: @autoclosure () -> Instant, + function: String, + file fileID: String, + line: UInt + ) -> any Tracing.Span { NoOpSpan(context: context()) } diff --git a/Sources/Tracing/SpanProtocol.swift b/Sources/Tracing/SpanProtocol.swift index 1c0c27a1..3b919093 100644 --- a/Sources/Tracing/SpanProtocol.swift +++ b/Sources/Tracing/SpanProtocol.swift @@ -79,9 +79,11 @@ public protocol Span: _SwiftTracingSendableSpan { /// - error: The error to be recorded /// - attributes: Additional attributes describing the error /// - instant: the time instant at which the event occurred - func recordError(_ error: Error, - attributes: SpanAttributes, - at instant: @autoclosure () -> Instant) + func recordError( + _ error: Error, + attributes: SpanAttributes, + at instant: @autoclosure () -> Instant + ) /// The attributes describing this `Span`. var attributes: SpanAttributes { @@ -185,18 +187,20 @@ public struct SpanEvent: Equatable { /// - name: The human-readable name of this event. /// - attributes: attributes describing this event. Defaults to no attributes. /// - instant: the time instant at which the event occurred - public init(name: String, - at instant: @autoclosure () -> Instant, - attributes: SpanAttributes = [:]) - { + public init( + name: String, + at instant: @autoclosure () -> Instant, + attributes: SpanAttributes = [:] + ) { self.name = name self.attributes = attributes self.nanosecondsSinceEpoch = instant().nanosecondsSinceEpoch } - public init(name: String, - attributes: SpanAttributes = [:]) - { + public init( + name: String, + attributes: SpanAttributes = [:] + ) { self.name = name self.attributes = attributes self.nanosecondsSinceEpoch = DefaultTracerClock.now.nanosecondsSinceEpoch diff --git a/Sources/Tracing/Tracer.swift b/Sources/Tracing/Tracer.swift index 4413f6c8..d9389dc0 100644 --- a/Sources/Tracing/Tracer.swift +++ b/Sources/Tracing/Tracer.swift @@ -350,6 +350,53 @@ public func withSpan( } } +#if swift(>=5.7.0) +/// Start a new ``Span`` and automatically end when the `operation` completes, +/// including recording the `error` in case the operation throws. +/// +/// The current task-local `ServiceContext` is picked up and provided to the underlying tracer. +/// It is also possible to pass a specific `context` explicitly, in which case attempting +/// to pick up the task-local context is prevented. This can be useful when we know that +/// we're about to start a top-level span, or if a span should be started from a different, +/// stored away previously, +/// +/// - Warning: You MUST NOT ``Span/end()`` the span explicitly, because at the end of the `withSpan` +/// operation closure returning the span will be closed automatically. +/// +/// - Parameters: +/// - operationName: The name of the operation being traced. This may be a handler function, database call, ... +/// - context: The `ServiceContext` providing information on where to start the new ``Span``. +/// - kind: The ``SpanKind`` of the new ``Span``. +/// - function: The function name in which the span was started +/// - fileID: The `fileID` where the span was started. +/// - line: The file line where the span was started. +/// - operation: The operation that this span should be measuring +/// - Returns: the value returned by `operation` +/// - Throws: the error the `operation` has thrown (if any) +@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext +@preconcurrency +public func withSpan( + _ operationName: String, + context: @Sendable @autoclosure () -> ServiceContext = .current ?? .topLevel, + ofKind kind: SpanKind = .internal, + function: String = #function, + file fileID: String = #fileID, + line: UInt = #line, + _ operation: @Sendable (any Span) async throws -> T +) async rethrows -> T { + try await InstrumentationSystem.legacyTracer.withAnySpan( + operationName, + at: DefaultTracerClock.now, + context: context(), + ofKind: kind, + function: function, + file: fileID, + line: line + ) { anySpan in + try await operation(anySpan) + } +} +#else /// Start a new ``Span`` and automatically end when the `operation` completes, /// including recording the `error` in case the operation throws. /// @@ -394,6 +441,7 @@ public func withSpan( try await operation(anySpan) } } +#endif #if swift(>=5.7.0) /// Start a new ``Span`` and automatically end when the `operation` completes, @@ -420,6 +468,7 @@ public func withSpan( /// - Returns: the value returned by `operation` /// - Throws: the error the `operation` has thrown (if any) @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) +@preconcurrency public func withSpan( _ operationName: String, context: @autoclosure () -> ServiceContext = .current ?? .topLevel, @@ -428,7 +477,7 @@ public func withSpan( function: String = #function, file fileID: String = #fileID, line: UInt = #line, - _ operation: (any Span) async throws -> T + _ operation: @Sendable (any Span) async throws -> T ) async rethrows -> T { try await InstrumentationSystem.legacyTracer.withAnySpan( operationName, diff --git a/Sources/_TracingBenchmarkTools/ArgParser.swift b/Sources/_TracingBenchmarkTools/ArgParser.swift index 518defb6..c49e1031 100644 --- a/Sources/_TracingBenchmarkTools/ArgParser.swift +++ b/Sources/_TracingBenchmarkTools/ArgParser.swift @@ -25,6 +25,7 @@ //===----------------------------------------------------------------------===// import Foundation +@_spi(Locking) import Instrumentation enum ArgumentError: Error { case missingValue(String) @@ -81,7 +82,7 @@ class ArgumentParser { private var arguments: [Argument] = [] private let programName: String = { // Strip full path from the program name. - let r = CommandLine.arguments[0].reversed() + let r = ProcessInfo.processInfo.arguments[0].reversed() let ss = r[r.startIndex ..< (r.firstIndex(of: "/") ?? r.endIndex)] return String(ss.reversed()) }() @@ -150,10 +151,14 @@ class ArgumentParser { try self.arguments.forEach { try $0.apply() } // type-check and store values return self.result } catch let error as ArgumentError { - fputs("error: \(error)\n", stderr) + lockedStderr.withValue { stderr in + _ = fputs("error: \(error)\n", stderr) + } exit(1) } catch { - fflush(stdout) + lockedStdout.withValue { stdout in + _ = fflush(stdout) + } fatalError("\(error)") } } @@ -173,7 +178,8 @@ class ArgumentParser { /// the supported argument syntax. private func parseArgs() throws { // For each argument we are passed... - for arg in CommandLine.arguments[1 ..< CommandLine.arguments.count] { + let arguments = ProcessInfo.processInfo.arguments + for arg in arguments[1 ..< arguments.count] { // If the argument doesn't match the optional argument pattern. Add // it to the positional argument list and continue... if !arg.starts(with: "-") { diff --git a/Sources/_TracingBenchmarkTools/BenchmarkCategory.swift b/Sources/_TracingBenchmarkTools/BenchmarkCategory.swift index 174fa28b..4acdaf39 100644 --- a/Sources/_TracingBenchmarkTools/BenchmarkCategory.swift +++ b/Sources/_TracingBenchmarkTools/BenchmarkCategory.swift @@ -18,7 +18,7 @@ // //===----------------------------------------------------------------------===// -public enum BenchmarkCategory: String { +public enum BenchmarkCategory: String, Sendable { // Most benchmarks are assumed to be "stable" and will be regularly tracked at // each commit. A handful may be marked unstable if continually tracking them is // counterproductive. diff --git a/Sources/_TracingBenchmarkTools/BenchmarkTools.swift b/Sources/_TracingBenchmarkTools/BenchmarkTools.swift index 1a906eef..003d6b0e 100644 --- a/Sources/_TracingBenchmarkTools/BenchmarkTools.swift +++ b/Sources/_TracingBenchmarkTools/BenchmarkTools.swift @@ -23,6 +23,8 @@ import Glibc #else import Darwin #endif +import Foundation +@_spi(Locking) import Instrumentation extension BenchmarkCategory: CustomStringConvertible { public var description: String { @@ -36,7 +38,7 @@ extension BenchmarkCategory: Comparable { } } -public struct BenchmarkPlatformSet: OptionSet { +public struct BenchmarkPlatformSet: OptionSet, Sendable { public let rawValue: Int public init(rawValue: Int) { @@ -59,12 +61,12 @@ public struct BenchmarkPlatformSet: OptionSet { } } -public struct BenchmarkInfo { +public struct BenchmarkInfo: Sendable { /// The name of the benchmark that should be displayed by the harness. public var name: String /// Shadow static variable for runFunction. - private var _runFunction: (Int) -> Void + private var _runFunction: @Sendable (Int) -> Void /// A function that invokes the specific benchmark routine. public var runFunction: ((Int) -> Void)? { @@ -83,7 +85,7 @@ public struct BenchmarkInfo { private var unsupportedPlatforms: BenchmarkPlatformSet /// Shadow variable for setUpFunction. - private var _setUpFunction: (() -> Void)? + private var _setUpFunction: (@Sendable () -> Void)? /// An optional function that if non-null is run before benchmark samples /// are timed. @@ -95,7 +97,7 @@ public struct BenchmarkInfo { } /// Shadow static variable for computed property tearDownFunction. - private var _tearDownFunction: (() -> Void)? + private var _tearDownFunction: (@Sendable () -> Void)? /// An optional function that if non-null is run after samples are taken. public var tearDownFunction: (() -> Void)? { @@ -108,9 +110,9 @@ public struct BenchmarkInfo { public var legacyFactor: Int? public init( - name: String, runFunction: @escaping (Int) -> Void, tags: [BenchmarkCategory], - setUpFunction: (() -> Void)? = nil, - tearDownFunction: (() -> Void)? = nil, + name: String, runFunction: @escaping @Sendable (Int) -> Void, tags: [BenchmarkCategory], + setUpFunction: (@Sendable () -> Void)? = nil, + tearDownFunction: (@Sendable () -> Void)? = nil, unsupportedPlatforms: BenchmarkPlatformSet = [], legacyFactor: Int? = nil ) { @@ -168,17 +170,28 @@ struct LFSR { } } -var lfsrRandomGenerator = LFSR() +let lfsrRandomGenerator: LockedValueBox = .init(LFSR()) // Start the generator from the beginning public func SRand() { - lfsrRandomGenerator = LFSR() + lfsrRandomGenerator.withValue { lfsr in + lfsr = LFSR() + } } public func Random() -> Int64 { - lfsrRandomGenerator.randInt() + lfsrRandomGenerator.withValue { lfsr in + lfsr.randInt() + } } +// Can't access stdout/stderr directly in strict concurrency checking mode. +// let lockedStdout = LockedValueBox(stdout) +// let lockedStderr = LockedValueBox(stderr) + +let lockedStdout = LockedValueBox(fdopen(STDOUT_FILENO, "w")) +let lockedStderr = LockedValueBox(fdopen(STDERR_FILENO, "w")) + @inlinable @inline(__always) public func CheckResults( diff --git a/Sources/_TracingBenchmarkTools/DriverUtils.swift b/Sources/_TracingBenchmarkTools/DriverUtils.swift index 8c333663..e45b30fa 100644 --- a/Sources/_TracingBenchmarkTools/DriverUtils.swift +++ b/Sources/_TracingBenchmarkTools/DriverUtils.swift @@ -30,6 +30,7 @@ import Glibc import Darwin // import LibProc #endif +@_spi(Locking) import Instrumentation public struct BenchResults { typealias T = Int @@ -63,7 +64,12 @@ public struct BenchResults { var median: T { self[0.5] } } -public var registeredBenchmarks: [BenchmarkInfo] = [] +let registeredBenchmarks: LockedValueBox<[BenchmarkInfo]> = .init([]) +public func internalRegisterBenchmark(_ benchmark: BenchmarkInfo) { + registeredBenchmarks.withValue { benchmarks in + benchmarks.append(benchmark) + } +} enum TestAction { case run @@ -136,8 +142,8 @@ struct TestConfig { // We support specifying multiple tags by splitting on comma, i.e.: // --tags=Array,Dictionary // --skip-tags=Array,Set,unstable,skip - Set( - try tags.split(separator: ",").map(String.init).map { + try Set( + tags.split(separator: ",").map(String.init).map { try checked({ BenchmarkCategory(rawValue: $0) }, $0) } ) @@ -687,7 +693,9 @@ final class TestRunner { ).joined(separator: c.delim) print(benchmarkStats) - fflush(stdout) + lockedStdout.withValue { stdout in + _ = fflush(stdout) + } if results != nil { testCount += 1 @@ -705,7 +713,7 @@ final class TestRunner { } public func main() { - let config = TestConfig(registeredBenchmarks) + let config = TestConfig(registeredBenchmarks.withValue { $0 }) switch config.action { case .listTests: print("#\(config.delim)Test\(config.delim)[Tags]") diff --git a/Sources/_TracingBenchmarks/SpanAttributesDSLBenchmark.swift b/Sources/_TracingBenchmarks/SpanAttributesDSLBenchmark.swift index 43b9d700..db460c11 100644 --- a/Sources/_TracingBenchmarks/SpanAttributesDSLBenchmark.swift +++ b/Sources/_TracingBenchmarks/SpanAttributesDSLBenchmark.swift @@ -14,6 +14,7 @@ import _TracingBenchmarkTools import Tracing +@_spi(Locking) import Instrumentation @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext enum DSLBenchmarks { @@ -23,21 +24,21 @@ enum DSLBenchmarks { runFunction: { _ in try! bench_empty(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( name: "SpanAttributesDSLBenchmarks.001_bench_makeSpan", runFunction: { _ in try! bench_makeSpan(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( name: "SpanAttributesDSLBenchmarks.002_bench_startSpan_end", runFunction: { _ in try! bench_makeSpan(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( @@ -45,14 +46,14 @@ enum DSLBenchmarks { runFunction: { _ in try! bench_set_String_raw(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( name: "SpanAttributesDSLBenchmarks.01_bench_set_String_dsl", runFunction: { _ in try! bench_set_String_dsl(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( @@ -60,25 +61,37 @@ enum DSLBenchmarks { runFunction: { _ in try! bench_set_String_raw(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), BenchmarkInfo( name: "SpanAttributesDSLBenchmarks.03_bench_set_Int_dsl", runFunction: { _ in try! bench_set_String_dsl(times: 100) }, tags: [], setUpFunction: { setUp() }, - tearDownFunction: tearDown + tearDownFunction: { tearDown() } ), ] - fileprivate static var span: (any Tracing.Span)! + fileprivate static let span: LockedValueBox<(any Tracing.Span)?> = .init(nil) + + fileprivate static func runTimesWithSpan(_ times: Int, work: (any Tracing.Span) -> Void) { + self.span.withValue { span in + for _ in 0 ..< times { + work(span!) + } + } + } fileprivate static func setUp() { - self.span = InstrumentationSystem.legacyTracer.startAnySpan("something", context: .topLevel) + self.span.withValue { span in + span = InstrumentationSystem.legacyTracer.startAnySpan("something", context: .topLevel) + } } fileprivate static func tearDown() { - self.span = nil + self.span.withValue { span in + span = nil + } } // ==== ---------------------------------------------------------------------------------------------------------------- @@ -104,14 +117,14 @@ enum DSLBenchmarks { // MARK: set String static func bench_set_String_raw(times: Int) throws { - for _ in 0 ..< times { - self.span.attributes["http.method"] = "POST" + self.runTimesWithSpan(times) { span in + span.attributes["http.method"] = "POST" } } static func bench_set_String_dsl(times: Int) throws { - for _ in 0 ..< times { - self.span.attributes.http.method = "POST" + self.runTimesWithSpan(times) { span in + span.attributes.http.method = "POST" } } @@ -119,14 +132,14 @@ enum DSLBenchmarks { // MARK: set Int static func bench_set_Int_raw(times: Int) throws { - for _ in 0 ..< times { - self.span.attributes["http.status_code"] = 200 + self.runTimesWithSpan(times) { span in + span.attributes["http.status_code"] = 200 } } static func bench_set_Int_dsl(times: Int) throws { - for _ in 0 ..< times { - self.span.attributes.http.statusCode = 200 + self.runTimesWithSpan(times) { span in + span.attributes.http.statusCode = 200 } } diff --git a/Sources/_TracingBenchmarks/main.swift b/Sources/_TracingBenchmarks/main.swift index 73ad3c5a..778602ca 100644 --- a/Sources/_TracingBenchmarks/main.swift +++ b/Sources/_TracingBenchmarks/main.swift @@ -24,7 +24,7 @@ assert({ @inline(__always) private func registerBenchmark(_ bench: BenchmarkInfo) { - registeredBenchmarks.append(bench) + internalRegisterBenchmark(bench) } @inline(__always) @@ -33,7 +33,7 @@ private func registerBenchmark(_ benches: [BenchmarkInfo]) { } @inline(__always) -private func registerBenchmark(_ name: String, _ function: @escaping (Int) -> Void, _ tags: [BenchmarkCategory]) { +private func registerBenchmark(_ name: String, _ function: @escaping @Sendable (Int) -> Void, _ tags: [BenchmarkCategory]) { registerBenchmark(BenchmarkInfo(name: name, runFunction: function, tags: tags)) } diff --git a/Tests/TracingTests/ActorTracingTests.swift b/Tests/TracingTests/ActorTracingTests.swift index 1e88f862..b76172b2 100644 --- a/Tests/TracingTests/ActorTracingTests.swift +++ b/Tests/TracingTests/ActorTracingTests.swift @@ -12,7 +12,7 @@ // //===----------------------------------------------------------------------===// -@testable import Instrumentation +@testable @_spi(Locking) import Instrumentation import ServiceContextModule import Tracing import XCTest @@ -27,13 +27,17 @@ final class ActorTracingTests: XCTestCase { func work() async {} actor Foo { - var bar = 0 + let bar: LockedValueBox = .init(0) func foo() async { - var num = 0 + let num: LockedValueBox = .init(0) await withSpan(#function) { _ in - bar += 1 + self.bar.withValue { bar in + bar += 1 + } await work() - num += 1 + num.withValue { num in + num += 1 + } } } } diff --git a/Tests/TracingTests/DynamicTracepointTracerTests.swift b/Tests/TracingTests/DynamicTracepointTracerTests.swift index 122a3d29..53e20de4 100644 --- a/Tests/TracingTests/DynamicTracepointTracerTests.swift +++ b/Tests/TracingTests/DynamicTracepointTracerTests.swift @@ -281,14 +281,15 @@ extension DynamicTracepointTestTracer { return span } - init(operationName: String, - startTime: Instant, - context: ServiceContext, - kind: SpanKind, - file fileID: String, - line: UInt, - onEnd: @escaping (TracepointSpan) -> Void) - { + init( + operationName: String, + startTime: Instant, + context: ServiceContext, + kind: SpanKind, + file fileID: String, + line: UInt, + onEnd: @escaping (TracepointSpan) -> Void + ) { self.operationName = operationName self.startTimestampNanosSinceEpoch = startTime.nanosecondsSinceEpoch self.context = context @@ -333,14 +334,15 @@ extension DynamicTracepointTestTracer { extension DynamicTracepointTestTracer: Tracer { typealias Span = TracepointSpan - func startSpan(_ operationName: String, - context: @autoclosure () -> ServiceContext, - ofKind kind: Tracing.SpanKind, - at instant: @autoclosure () -> Instant, - function: String, - file fileID: String, - line: UInt) -> TracepointSpan - { + func startSpan( + _ operationName: String, + context: @autoclosure () -> ServiceContext, + ofKind kind: Tracing.SpanKind, + at instant: @autoclosure () -> Instant, + function: String, + file fileID: String, + line: UInt + ) -> TracepointSpan { let tracepoint = TracepointID(function: function, fileID: fileID, line: line) guard self.shouldRecord(tracepoint: tracepoint) else { return TracepointSpan.notRecording(file: fileID, line: line) diff --git a/Tests/TracingTests/TracedLock.swift b/Tests/TracingTests/TracedLock.swift index 6a641e1e..6ea07330 100644 --- a/Tests/TracingTests/TracedLock.swift +++ b/Tests/TracingTests/TracedLock.swift @@ -17,11 +17,13 @@ import Instrumentation import ServiceContextModule import Tracing -final class TracedLock { - let name: String - let underlyingLock: NSLock +/// Marked as @unchecked Sendable due to the synchronization being +/// performed manually using locks. +final class TracedLock: @unchecked Sendable { + private let name: String + private let underlyingLock: NSLock - var activeSpan: (any Tracing.Span)? + private var activeSpan: (any Tracing.Span)? init(name: String) { self.name = name diff --git a/Tests/TracingTests/TracerTests.swift b/Tests/TracingTests/TracerTests.swift index a6cfeffd..5f0b556b 100644 --- a/Tests/TracingTests/TracerTests.swift +++ b/Tests/TracingTests/TracerTests.swift @@ -12,10 +12,13 @@ // //===----------------------------------------------------------------------===// -@testable import Instrumentation +@testable @_spi(Locking) import Instrumentation import ServiceContextModule import Tracing import XCTest +#if os(Linux) +@preconcurrency import Dispatch +#endif final class TracerTests: XCTestCase { override class func tearDown() { @@ -179,21 +182,21 @@ final class TracerTests: XCTestCase { InstrumentationSystem.bootstrapInternal(nil) } - var spanEnded = false - tracer.onEndSpan = { _ in spanEnded = true } + let spanEnded: LockedValueBox = .init(false) + tracer.onEndSpan = { _ in spanEnded.withValue { $0 = true } } - func operation(span: any Tracing.Span) async throws -> String { + let operation: @Sendable (any Tracing.Span) async throws -> String = { _ in "world" } try self.testAsync { let value = try await tracer.withAnySpan("hello") { (span: any Tracing.Span) -> String in XCTAssertEqual(span.context.traceID, ServiceContext.current?.traceID) - return try await operation(span: span) + return try await operation(span) } XCTAssertEqual(value, "world") - XCTAssertTrue(spanEnded) + XCTAssertTrue(spanEnded.withValue { $0 }) } #endif } @@ -209,10 +212,10 @@ final class TracerTests: XCTestCase { InstrumentationSystem.bootstrapInternal(nil) } - var spanEnded = false - tracer.onEndSpan = { _ in spanEnded = true } + let spanEnded: LockedValueBox = .init(false) + tracer.onEndSpan = { _ in spanEnded.withValue { $0 = true } } - func operation(span: any Tracing.Span) async -> String { + let operation: @Sendable (any Tracing.Span) async -> String = { _ in "world" } @@ -222,11 +225,11 @@ final class TracerTests: XCTestCase { let value = await tracer.withAnySpan("hello", context: fromNonAsyncWorld) { (span: any Tracing.Span) -> String in XCTAssertEqual(span.context.traceID, ServiceContext.current?.traceID) XCTAssertEqual(span.context.traceID, fromNonAsyncWorld.traceID) - return await operation(span: span) + return await operation(span) } XCTAssertEqual(value, "world") - XCTAssertTrue(spanEnded) + XCTAssertTrue(spanEnded.withValue { $0 }) } } @@ -241,10 +244,10 @@ final class TracerTests: XCTestCase { InstrumentationSystem.bootstrapInternal(nil) } - var spanEnded = false - tracer.onEndSpan = { _ in spanEnded = true } + let spanEnded: LockedValueBox = .init(false) + tracer.onEndSpan = { _ in spanEnded.withValue { $0 = true } } - func operation(span: any Tracing.Span) async throws -> String { + let operation: @Sendable (any Tracing.Span) async throws -> String = { _ in throw ExampleSpanError() } @@ -252,7 +255,7 @@ final class TracerTests: XCTestCase { do { _ = try await tracer.withAnySpan("hello", operation) } catch { - XCTAssertTrue(spanEnded) + XCTAssertTrue(spanEnded.withValue { $0 }) XCTAssertEqual(error as? ExampleSpanError, ExampleSpanError()) return } @@ -271,10 +274,10 @@ final class TracerTests: XCTestCase { InstrumentationSystem.bootstrapInternal(nil) } - var spanEnded = false - tracer.onEndSpan = { _ in spanEnded = true } + let spanEnded: LockedValueBox = .init(false) + tracer.onEndSpan = { _ in spanEnded.withValue { $0 = true } } - func operation(span: any Tracing.Span) async throws -> String { + let operation: @Sendable (any Tracing.Span) async throws -> String = { _ in throw ExampleSpanError() } @@ -282,7 +285,7 @@ final class TracerTests: XCTestCase { do { _ = try await withSpan("hello", operation) } catch { - XCTAssertTrue(spanEnded) + XCTAssertTrue(spanEnded.withValue { $0 }) XCTAssertEqual(error as? ExampleSpanError, ExampleSpanError()) return } @@ -301,18 +304,18 @@ final class TracerTests: XCTestCase { InstrumentationSystem.bootstrapInternal(nil) } - var spanEnded = false - tracer.onEndSpan = { _ in spanEnded = true } + let spanEnded: LockedValueBox = .init(false) + tracer.onEndSpan = { _ in spanEnded.withValue { $0 = true } } - func operation(span: any Tracing.Span) throws -> String { + let operation: @Sendable (any Tracing.Span) async throws -> String = { _ in throw ExampleSpanError() } self.testAsync { do { - _ = try withSpan("hello", operation) + _ = try await withSpan("hello", operation) } catch { - XCTAssertTrue(spanEnded) + XCTAssertTrue(spanEnded.withValue { $0 }) XCTAssertEqual(error as? ExampleSpanError, ExampleSpanError()) return } @@ -370,7 +373,7 @@ final class TracerTests: XCTestCase { // @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *) /// Helper method to execute async operations until we can use async tests (currently incompatible with the generated LinuxMain file). /// - Parameter operation: The operation to test. - func testAsync(_ operation: @escaping () async throws -> Void) rethrows { + func testAsync(_ operation: @Sendable @escaping () async throws -> Void) rethrows { let group = DispatchGroup() group.enter() Task.detached { diff --git a/docker/docker-compose.2004.58.yaml b/docker/docker-compose.2004.58.yaml index 75ed2cc4..9f52c280 100644 --- a/docker/docker-compose.2004.58.yaml +++ b/docker/docker-compose.2004.58.yaml @@ -13,6 +13,7 @@ services: image: swift-distributed-tracing:20.04-5.8 environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery + - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:20.04-5.8 diff --git a/docker/docker-compose.2204.59.yaml b/docker/docker-compose.2204.59.yaml index 11bfb47e..dc1c58e7 100644 --- a/docker/docker-compose.2204.59.yaml +++ b/docker/docker-compose.2204.59.yaml @@ -12,6 +12,7 @@ services: image: swift-distributed-tracing:22.04-5.9 environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery + - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:22.04-5.9 diff --git a/docker/docker-compose.2204.main.yaml b/docker/docker-compose.2204.main.yaml index f67b0ddd..a863b665 100644 --- a/docker/docker-compose.2204.main.yaml +++ b/docker/docker-compose.2204.main.yaml @@ -12,6 +12,7 @@ services: image: swift-distributed-tracing:22.04-main environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery + - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:22.04-main diff --git a/docker/docker-compose.yaml b/docker/docker-compose.yaml index d9d26ad0..d1277a58 100644 --- a/docker/docker-compose.yaml +++ b/docker/docker-compose.yaml @@ -28,7 +28,7 @@ services: test: <<: *common - command: /bin/bash -xcl "swift test -Xswiftc -warnings-as-errors $${FORCE_TEST_DISCOVERY-} $${SANITIZER_ARG-}" + command: /bin/bash -xcl "swift test -Xswiftc -warnings-as-errors $${FORCE_TEST_DISCOVERY-} $${SANITIZER_ARG-} $${STRICT_CONCURRENCY_ARG-}" # util From 14ab72180a12a7895bdefbcce4cbabd833cbef82 Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Mon, 14 Aug 2023 12:16:43 +0200 Subject: [PATCH 2/3] Undo making operation closures sendable --- Sources/Tracing/Tracer.swift | 51 +--------------------- Tests/TracingTests/ActorTracingTests.swift | 14 +++--- 2 files changed, 6 insertions(+), 59 deletions(-) diff --git a/Sources/Tracing/Tracer.swift b/Sources/Tracing/Tracer.swift index d9389dc0..4413f6c8 100644 --- a/Sources/Tracing/Tracer.swift +++ b/Sources/Tracing/Tracer.swift @@ -350,53 +350,6 @@ public func withSpan( } } -#if swift(>=5.7.0) -/// Start a new ``Span`` and automatically end when the `operation` completes, -/// including recording the `error` in case the operation throws. -/// -/// The current task-local `ServiceContext` is picked up and provided to the underlying tracer. -/// It is also possible to pass a specific `context` explicitly, in which case attempting -/// to pick up the task-local context is prevented. This can be useful when we know that -/// we're about to start a top-level span, or if a span should be started from a different, -/// stored away previously, -/// -/// - Warning: You MUST NOT ``Span/end()`` the span explicitly, because at the end of the `withSpan` -/// operation closure returning the span will be closed automatically. -/// -/// - Parameters: -/// - operationName: The name of the operation being traced. This may be a handler function, database call, ... -/// - context: The `ServiceContext` providing information on where to start the new ``Span``. -/// - kind: The ``SpanKind`` of the new ``Span``. -/// - function: The function name in which the span was started -/// - fileID: The `fileID` where the span was started. -/// - line: The file line where the span was started. -/// - operation: The operation that this span should be measuring -/// - Returns: the value returned by `operation` -/// - Throws: the error the `operation` has thrown (if any) -@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) // for TaskLocal ServiceContext -@preconcurrency -public func withSpan( - _ operationName: String, - context: @Sendable @autoclosure () -> ServiceContext = .current ?? .topLevel, - ofKind kind: SpanKind = .internal, - function: String = #function, - file fileID: String = #fileID, - line: UInt = #line, - _ operation: @Sendable (any Span) async throws -> T -) async rethrows -> T { - try await InstrumentationSystem.legacyTracer.withAnySpan( - operationName, - at: DefaultTracerClock.now, - context: context(), - ofKind: kind, - function: function, - file: fileID, - line: line - ) { anySpan in - try await operation(anySpan) - } -} -#else /// Start a new ``Span`` and automatically end when the `operation` completes, /// including recording the `error` in case the operation throws. /// @@ -441,7 +394,6 @@ public func withSpan( try await operation(anySpan) } } -#endif #if swift(>=5.7.0) /// Start a new ``Span`` and automatically end when the `operation` completes, @@ -468,7 +420,6 @@ public func withSpan( /// - Returns: the value returned by `operation` /// - Throws: the error the `operation` has thrown (if any) @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) -@preconcurrency public func withSpan( _ operationName: String, context: @autoclosure () -> ServiceContext = .current ?? .topLevel, @@ -477,7 +428,7 @@ public func withSpan( function: String = #function, file fileID: String = #fileID, line: UInt = #line, - _ operation: @Sendable (any Span) async throws -> T + _ operation: (any Span) async throws -> T ) async rethrows -> T { try await InstrumentationSystem.legacyTracer.withAnySpan( operationName, diff --git a/Tests/TracingTests/ActorTracingTests.swift b/Tests/TracingTests/ActorTracingTests.swift index b76172b2..1e88f862 100644 --- a/Tests/TracingTests/ActorTracingTests.swift +++ b/Tests/TracingTests/ActorTracingTests.swift @@ -12,7 +12,7 @@ // //===----------------------------------------------------------------------===// -@testable @_spi(Locking) import Instrumentation +@testable import Instrumentation import ServiceContextModule import Tracing import XCTest @@ -27,17 +27,13 @@ final class ActorTracingTests: XCTestCase { func work() async {} actor Foo { - let bar: LockedValueBox = .init(0) + var bar = 0 func foo() async { - let num: LockedValueBox = .init(0) + var num = 0 await withSpan(#function) { _ in - self.bar.withValue { bar in - bar += 1 - } + bar += 1 await work() - num.withValue { num in - num += 1 - } + num += 1 } } } From f9211ed3074e8e3cc492e5eacf1dc9d876d415e7 Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Mon, 14 Aug 2023 13:11:41 +0200 Subject: [PATCH 3/3] Disable strict concurrency checking in CI for now --- docker/docker-compose.2004.58.yaml | 2 +- docker/docker-compose.2204.59.yaml | 2 +- docker/docker-compose.2204.main.yaml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docker/docker-compose.2004.58.yaml b/docker/docker-compose.2004.58.yaml index 9f52c280..26ce760b 100644 --- a/docker/docker-compose.2004.58.yaml +++ b/docker/docker-compose.2004.58.yaml @@ -13,7 +13,7 @@ services: image: swift-distributed-tracing:20.04-5.8 environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery - - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete + # - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:20.04-5.8 diff --git a/docker/docker-compose.2204.59.yaml b/docker/docker-compose.2204.59.yaml index dc1c58e7..2da890c2 100644 --- a/docker/docker-compose.2204.59.yaml +++ b/docker/docker-compose.2204.59.yaml @@ -12,7 +12,7 @@ services: image: swift-distributed-tracing:22.04-5.9 environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery - - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete + # - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:22.04-5.9 diff --git a/docker/docker-compose.2204.main.yaml b/docker/docker-compose.2204.main.yaml index a863b665..fe574376 100644 --- a/docker/docker-compose.2204.main.yaml +++ b/docker/docker-compose.2204.main.yaml @@ -12,7 +12,7 @@ services: image: swift-distributed-tracing:22.04-main environment: - FORCE_TEST_DISCOVERY=--enable-test-discovery - - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete + # - STRICT_CONCURRENCY_ARG=-Xswiftc -strict-concurrency=complete shell: image: swift-distributed-tracing:22.04-main