From fd0ee6956bd82ad7dd995550443327cf52921bad Mon Sep 17 00:00:00 2001 From: tomer doron Date: Wed, 29 Jun 2022 11:08:12 -0700 Subject: [PATCH] better abstration for MetricsSystem state (#112) motivation: prepare for sendable checks (they dont work well with static state) changes: * abstract the MetricsSystem state into a "boxed" class that handles the locking * adjust call sites --- Sources/CoreMetrics/Locks.swift | 144 +++++++++++++++++++++++------- Sources/CoreMetrics/Metrics.swift | 60 ++++++++----- 2 files changed, 152 insertions(+), 52 deletions(-) diff --git a/Sources/CoreMetrics/Locks.swift b/Sources/CoreMetrics/Locks.swift index f90c7a7..f5e1825 100644 --- a/Sources/CoreMetrics/Locks.swift +++ b/Sources/CoreMetrics/Locks.swift @@ -26,29 +26,54 @@ // //===----------------------------------------------------------------------===// -#if os(macOS) || os(iOS) || os(tvOS) || os(watchOS) +#if canImport(WASILibc) +// No locking on WASILibc +#elseif canImport(Darwin) import Darwin -#else +#elseif os(Windows) +import WinSDK +#elseif canImport(Glibc) import Glibc +#else +#error("Unsupported runtime") #endif /// A threading lock based on `libpthread` instead of `libdispatch`. /// /// 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. +/// one used by NIO. On Windows, the lock is based on the substantially similar +/// `SRWLOCK` type. internal final class Lock { - private let mutex: UnsafeMutablePointer = UnsafeMutablePointer.allocate(capacity: 1) + #if os(Windows) + fileprivate let mutex: UnsafeMutablePointer = + UnsafeMutablePointer.allocate(capacity: 1) + #else + fileprivate let mutex: UnsafeMutablePointer = + UnsafeMutablePointer.allocate(capacity: 1) + #endif /// Create a new lock. public init() { - let err = pthread_mutex_init(self.mutex, nil) - precondition(err == 0, "pthread_mutex_init failed with error \(err)") + #if os(Windows) + InitializeSRWLock(self.mutex) + #else + var attr = pthread_mutexattr_t() + pthread_mutexattr_init(&attr) + pthread_mutexattr_settype(&attr, .init(PTHREAD_MUTEX_ERRORCHECK)) + + let err = pthread_mutex_init(self.mutex, &attr) + precondition(err == 0, "\(#function) failed in pthread_mutex with error \(err)") + #endif } deinit { + #if os(Windows) + // SRWLOCK does not need to be free'd + #else let err = pthread_mutex_destroy(self.mutex) - precondition(err == 0, "pthread_mutex_destroy failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_mutex with error \(err)") + #endif self.mutex.deallocate() } @@ -57,8 +82,12 @@ internal final class Lock { /// Whenever possible, consider using `withLock` instead of this method and /// `unlock`, to simplify lock handling. public func lock() { + #if os(Windows) + AcquireSRWLockExclusive(self.mutex) + #else let err = pthread_mutex_lock(self.mutex) - precondition(err == 0, "pthread_mutex_lock failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_mutex with error \(err)") + #endif } /// Release the lock. @@ -66,8 +95,12 @@ internal final class Lock { /// Whenever possible, consider using `withLock` instead of this method and /// `lock`, to simplify lock handling. public func unlock() { + #if os(Windows) + ReleaseSRWLockExclusive(self.mutex) + #else let err = pthread_mutex_unlock(self.mutex) - precondition(err == 0, "pthread_mutex_unlock failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_mutex with error \(err)") + #endif } } @@ -96,62 +129,109 @@ extension Lock { } } -/// A threading lock based on `libpthread` instead of `libdispatch`. +/// A reader/writer threading lock based on `libpthread` instead of `libdispatch`. /// -/// This object provides a lock on top of a single `pthread_mutex_t`. This kind +/// This object provides a lock on top of a single `pthread_rwlock_t`. This kind /// of lock is safe to use with `libpthread`-based threading models, such as the -/// one used by NIO. +/// one used by NIO. On Windows, the lock is based on the substantially similar +/// `SRWLOCK` type. internal final class ReadWriteLock { - private let rwlock: UnsafeMutablePointer = UnsafeMutablePointer.allocate(capacity: 1) + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + fileprivate let rwlock: UnsafeMutablePointer = + UnsafeMutablePointer.allocate(capacity: 1) + fileprivate var shared: Bool = true + #else + fileprivate let rwlock: UnsafeMutablePointer = + UnsafeMutablePointer.allocate(capacity: 1) + #endif /// Create a new lock. public init() { + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + InitializeSRWLock(self.rwlock) + #else let err = pthread_rwlock_init(self.rwlock, nil) - precondition(err == 0, "pthread_rwlock_init failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_rwlock with error \(err)") + #endif } deinit { + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + // SRWLOCK does not need to be free'd + #else let err = pthread_rwlock_destroy(self.rwlock) - precondition(err == 0, "pthread_rwlock_destroy failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_rwlock with error \(err)") + #endif self.rwlock.deallocate() } /// Acquire a reader lock. /// - /// Whenever possible, consider using `withLock` instead of this method and - /// `unlock`, to simplify lock handling. + /// Whenever possible, consider using `withReaderLock` instead of this + /// method and `unlock`, to simplify lock handling. public func lockRead() { + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + AcquireSRWLockShared(self.rwlock) + self.shared = true + #else let err = pthread_rwlock_rdlock(self.rwlock) - precondition(err == 0, "pthread_rwlock_rdlock failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_rwlock with error \(err)") + #endif } /// Acquire a writer lock. /// - /// Whenever possible, consider using `withLock` instead of this method and - /// `unlock`, to simplify lock handling. + /// Whenever possible, consider using `withWriterLock` instead of this + /// method and `unlock`, to simplify lock handling. public func lockWrite() { + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + AcquireSRWLockExclusive(self.rwlock) + self.shared = false + #else let err = pthread_rwlock_wrlock(self.rwlock) - precondition(err == 0, "pthread_rwlock_wrlock failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_rwlock with error \(err)") + #endif } /// Release the lock. /// - /// Whenever possible, consider using `withLock` instead of this method and - /// `lock`, to simplify lock handling. + /// Whenever possible, consider using `withReaderLock` and `withWriterLock` + /// instead of this method and `lockRead` and `lockWrite`, to simplify lock + /// handling. public func unlock() { + #if canImport(WASILibc) + // WASILibc is single threaded, provides no locks + #elseif os(Windows) + if self.shared { + ReleaseSRWLockShared(self.rwlock) + } else { + ReleaseSRWLockExclusive(self.rwlock) + } + #else let err = pthread_rwlock_unlock(self.rwlock) - precondition(err == 0, "pthread_rwlock_unlock failed with error \(err)") + precondition(err == 0, "\(#function) failed in pthread_rwlock with error \(err)") + #endif } } extension ReadWriteLock { /// Acquire the reader lock for the duration of the given block. /// - /// This convenience method should be preferred to `lock` and `unlock` in - /// most situations, as it ensures that the lock will be released regardless - /// of how `body` exits. + /// This convenience method should be preferred to `lockRead` and `unlock` + /// in most situations, as it ensures that the lock will be released + /// regardless of how `body` exits. /// - /// - Parameter body: The block to execute while holding the lock. + /// - Parameter body: The block to execute while holding the reader lock. /// - Returns: The value returned by the block. @inlinable internal func withReaderLock(_ body: () throws -> T) rethrows -> T { @@ -164,11 +244,11 @@ extension ReadWriteLock { /// Acquire the writer lock for the duration of the given block. /// - /// This convenience method should be preferred to `lock` and `unlock` in - /// most situations, as it ensures that the lock will be released regardless - /// of how `body` exits. + /// This convenience method should be preferred to `lockWrite` and `unlock` + /// in most situations, as it ensures that the lock will be released + /// regardless of how `body` exits. /// - /// - Parameter body: The block to execute while holding the lock. + /// - Parameter body: The block to execute while holding the writer lock. /// - Returns: The value returned by the block. @inlinable internal func withWriterLock(_ body: () throws -> T) rethrows -> T { diff --git a/Sources/CoreMetrics/Metrics.swift b/Sources/CoreMetrics/Metrics.swift index 1d571fd..06fa532 100644 --- a/Sources/CoreMetrics/Metrics.swift +++ b/Sources/CoreMetrics/Metrics.swift @@ -455,17 +455,7 @@ extension Timer: CustomStringConvertible { /// configured. `MetricsSystem` is set up just once in a given program to set up the desired metrics backend /// implementation. public enum MetricsSystem { - fileprivate static let lock = ReadWriteLock() - fileprivate static var _factory: MetricsFactory = NOOPMetricsHandler.instance - fileprivate static var initialized = false - - /// Acquire a writer lock for the duration of the given block. - /// - /// - Parameter body: The block to execute while holding the lock. - /// - Returns: The value returned by the block. - public static func withWriterLock(_ body: () throws -> T) rethrows -> T { - return try self.lock.withWriterLock(body) - } + private static let _factory = FactoryBox(NOOPMetricsHandler.instance) /// `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 @@ -474,23 +464,53 @@ public enum MetricsSystem { /// - parameters: /// - factory: A factory that given an identifier produces instances of metrics handlers such as `CounterHandler`, `RecorderHandler` and `TimerHandler`. public static func bootstrap(_ factory: MetricsFactory) { - self.lock.withWriterLock { - precondition(!self.initialized, "metrics system can only be initialized once per process. currently used factory: \(self._factory)") - self._factory = factory - self.initialized = true - } + self._factory.replaceFactory(factory, validate: true) } // for our testing we want to allow multiple bootstrapping internal static func bootstrapInternal(_ factory: MetricsFactory) { - self.lock.withWriterLock { - self._factory = factory - } + self._factory.replaceFactory(factory, validate: false) } /// Returns a reference to the configured factory. public static var factory: MetricsFactory { - return self.lock.withReaderLock { self._factory } + return self._factory.underlying + } + + /// Acquire a writer lock for the duration of the given block. + /// + /// - Parameter body: The block to execute while holding the lock. + /// - Returns: The value returned by the block. + public static func withWriterLock(_ body: () throws -> T) rethrows -> T { + return try self._factory.withWriterLock(body) + } + + private final class FactoryBox { + private let lock = ReadWriteLock() + fileprivate var _underlying: MetricsFactory + private var initialized = false + + init(_ underlying: MetricsFactory) { + self._underlying = underlying + } + + func replaceFactory(_ factory: MetricsFactory, validate: Bool) { + self.lock.withWriterLock { + precondition(!validate || !self.initialized, "metrics system can only be initialized once per process. currently used factory: \(self._underlying)") + self._underlying = factory + self.initialized = true + } + } + + var underlying: MetricsFactory { + return self.lock.withReaderLock { + return self._underlying + } + } + + func withWriterLock(_ body: () throws -> T) rethrows -> T { + return try self.lock.withWriterLock(body) + } } }