From a6466bfcacca3756565e3579898ea4ba0455e453 Mon Sep 17 00:00:00 2001 From: Brandon Sneed Date: Tue, 27 Jun 2023 11:08:24 -0700 Subject: [PATCH 1/2] Refactored Atomic implementation to avoid TSan warnings --- Sources/Segment/Utilities/Atomic.swift | 30 ++++++++----------- .../Policies/CountBasedFlushPolicy.swift | 2 +- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/Sources/Segment/Utilities/Atomic.swift b/Sources/Segment/Utilities/Atomic.swift index ab4e8c87..fe39dff7 100644 --- a/Sources/Segment/Utilities/Atomic.swift +++ b/Sources/Segment/Utilities/Atomic.swift @@ -7,29 +7,25 @@ import Foundation +// NOTE: Revised from previous implementation which used a struct and NSLock's. +// Thread Sanitizer was *correctly* capturing this issue, which was a little obscure +// given the property wrapper PLUS the semantics of a struct. Moving to `class` +// removes the semantics problem and lets TSan approve of what's happening. +// +// Additionally, moving to a lock free version is just desirable, so moved to a queue. +// + @propertyWrapper -public struct Atomic { - var value: T - private let lock = NSLock() +public class Atomic { + private var value: T + private let queue = DispatchQueue(label: "com.segment.atomic.\(UUID().uuidString)") public init(wrappedValue value: T) { self.value = value } public var wrappedValue: T { - get { return load() } - set { store(newValue: newValue) } - } - - func load() -> T { - lock.lock() - defer { lock.unlock() } - return value - } - - mutating func store(newValue: T) { - lock.lock() - defer { lock.unlock() } - value = newValue + get { return queue.sync { return value } } + set { queue.sync { value = newValue } } } } diff --git a/Sources/Segment/Utilities/Policies/CountBasedFlushPolicy.swift b/Sources/Segment/Utilities/Policies/CountBasedFlushPolicy.swift index a68abf07..2cadfa45 100644 --- a/Sources/Segment/Utilities/Policies/CountBasedFlushPolicy.swift +++ b/Sources/Segment/Utilities/Policies/CountBasedFlushPolicy.swift @@ -10,7 +10,7 @@ import Foundation public class CountBasedFlushPolicy: FlushPolicy { public weak var analytics: Analytics? internal var desiredCount: Int? - internal var count: Int = 0 + @Atomic internal var count: Int = 0 init() { } From 1e74776098347e9b098078631f1f69795c6266d1 Mon Sep 17 00:00:00 2001 From: Brandon Sneed Date: Tue, 27 Jun 2023 11:11:26 -0700 Subject: [PATCH 2/2] Additional reference --- Sources/Segment/Utilities/Atomic.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/Segment/Utilities/Atomic.swift b/Sources/Segment/Utilities/Atomic.swift index fe39dff7..50984aba 100644 --- a/Sources/Segment/Utilities/Atomic.swift +++ b/Sources/Segment/Utilities/Atomic.swift @@ -14,6 +14,7 @@ import Foundation // // Additionally, moving to a lock free version is just desirable, so moved to a queue. // +// Also see thread here: https://github.com/apple/swift-evolution/pull/1387 @propertyWrapper public class Atomic {