Skip to content

Commit

Permalink
Fix Sendability warnings (#137)
Browse files Browse the repository at this point in the history
* Fix Sendability warnings

* Apply suggestions from code review

* Fix another warning and @unknown default issue

---------

Co-authored-by: Konrad `ktoso` Malawski <[email protected]>
  • Loading branch information
gjcairo and ktoso authored Jun 21, 2024
1 parent d067b0e commit e0165b5
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 4 deletions.
4 changes: 4 additions & 0 deletions Sources/CoreMetrics/Locks.swift
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ extension Lock {
}
}

extension Lock: @unchecked Sendable {}

/// A reader/writer threading lock based on `libpthread` instead of `libdispatch`.
///
/// This object provides a lock on top of a single `pthread_rwlock_t`. This kind
Expand Down Expand Up @@ -273,3 +275,5 @@ extension ReadWriteLock {
try self.withWriterLock(body)
}
}

extension ReadWriteLock: @unchecked Sendable {}
8 changes: 5 additions & 3 deletions Sources/CoreMetrics/Metrics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,8 @@ public enum MetricsSystem {
return try self._factory.withWriterLock(body)
}

private final class FactoryBox {
// This can be `@unchecked Sendable` because we're manually gating access to mutable state with a lock.
private final class FactoryBox: @unchecked Sendable {
private let lock = ReadWriteLock()
fileprivate var _underlying: MetricsFactory
private var initialized = false
Expand Down Expand Up @@ -797,9 +798,10 @@ internal final class AccumulatingRoundingFloatingPointCounter: FloatingPointCoun
}

/// Wraps a RecorderHandler, adding support for incrementing values by storing an accumulated value and recording increments to the underlying CounterHandler after crossing integer boundaries.
internal final class AccumulatingMeter: MeterHandler {
/// - Note: we can annotate this class as `@unchecked Sendable` because we are manually gating access to mutable state (i.e., the `value` property) via a Lock.
internal final class AccumulatingMeter: MeterHandler, @unchecked Sendable {
private let recorderHandler: RecorderHandler
// FIXME: use atomics when available
// FIXME: use swift-atomics when floating point support is available
private var value: Double = 0
private let lock = Lock()

Expand Down
12 changes: 12 additions & 0 deletions Sources/Metrics/Metrics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ extension Timer {
/// - duration: The duration to record.
@inlinable
public func record(_ duration: DispatchTimeInterval) {
// This wrapping in a optional is a workaround because DispatchTimeInterval
// is a non-frozen public enum and Dispatch is built with library evolution
// mode turned on.
// This means we should have an `@unknown default` case, but this breaks
// on non-Darwin platforms.
// Switching over an optional means that the `.none` case will map to
// `default` (which means we'll always have a valid case to go into
// the default case), but in reality this case will never exist as this
// optional will never be nil.
let duration = Optional(duration)
switch duration {
case .nanoseconds(let value):
self.recordNanoseconds(value)
Expand All @@ -71,6 +81,8 @@ extension Timer {
self.recordSeconds(value)
case .never:
self.record(0)
default:
self.record(0)
}
}
}
Expand Down
14 changes: 13 additions & 1 deletion Tests/MetricsTests/MetricsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,17 @@ class MetricsExtensionsTests: XCTestCase {
// https://bugs.swift.org/browse/SR-6310
extension DispatchTimeInterval {
func nano() -> Int {
switch self {
// This wrapping in a optional is a workaround because DispatchTimeInterval
// is a non-frozen public enum and Dispatch is built with library evolution
// mode turned on.
// This means we should have an `@unknown default` case, but this breaks
// on non-Darwin platforms.
// Switching over an optional means that the `.none` case will map to
// `default` (which means we'll always have a valid case to go into
// the default case), but in reality this case will never exist as this
// optional will never be nil.
let interval = Optional(self)
switch interval {
case .nanoseconds(let value):
return value
case .microseconds(let value):
Expand All @@ -196,6 +206,8 @@ extension DispatchTimeInterval {
return value * 1_000_000_000
case .never:
return 0
default:
return 0
}
}
}

0 comments on commit e0165b5

Please sign in to comment.