Skip to content

Commit 8c1c080

Browse files
committed
Fix data race in @LazyInjected and @WeakLazyInjected on concurrent first access
1 parent a8a66ec commit 8c1c080

5 files changed

Lines changed: 198 additions & 39 deletions

File tree

CHANGELOG

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
# Factory Changelog
22

3+
## 3.0.4
4+
5+
* Fix data race in @LazyInjected and @WeakLazyInjected on concurrent first access #362
6+
* Fix potential CrossPlatformLock memory leak (missing deinit for heap-allocated lock storage) #362
7+
38
## 3.0.3
49

510
* Remove .dynamic from FactoryTesting package definition #363
@@ -128,7 +133,7 @@
128133

129134
## 2.3.1
130135

131-
* Fix for SpinLock on Linux
136+
* Fix for CrossPlatformLock on Linux
132137
* Add action to test linux - Issue #113
133138
* Add various aliases to avoid external naming conflicts - Issue #154
134139

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
A modern approach to Container-Based Dependency Injection for Swift and SwiftUI.
77

8-
## Factory Version 3.0.3
8+
## Factory Version 3.0.4
99

1010
Factory is strongly influenced by SwiftUI, and in my opinion is highly suited for that environment. Factory is...
1111

Sources/FactoryKit/FactoryKit/Injections.swift

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -113,50 +113,61 @@ extension Injected: @unchecked Sendable where T: Sendable {}
113113

114114
private var thunk: () -> Factory<T>
115115
private var dependency: T!
116-
private var initialize = true
117-
116+
private var initialized = false
117+
private let lock: CrossPlatformLock
118+
118119
/// Initializes the property wrapper. The dependency isn't resolved until the wrapped value is accessed for the first time.
119120
/// - Parameter keyPath: KeyPath to a Factory on the default Container.
120121
public init(_ keyPath: KeyPath<Container, Factory<T>>) {
122+
self.lock = CrossPlatformLock()
121123
self.thunk = { Container.shared[keyPath: keyPath] }
122124
}
123-
125+
124126
/// Initializes the property wrapper. The dependency isn't resolved until the wrapped value is accessed for the first time.
125127
/// - Parameter keyPath: KeyPath to a Factory on the specified Container.
126128
public init<C:SharedContainer>(_ keyPath: KeyPath<C, Factory<T>>) {
129+
self.lock = CrossPlatformLock()
127130
self.thunk = { C.shared[keyPath: keyPath] }
128131
}
129-
132+
130133
/// Manages the wrapped dependency, which is resolved when this value is accessed for the first time.
131134
public var wrappedValue: T {
132135
mutating get {
133-
if initialize {
134-
resolve()
136+
lock.withLock {
137+
if !initialized {
138+
dependency = thunk()()
139+
initialized = true
140+
}
141+
return dependency
135142
}
136-
return dependency
137143
}
138144
mutating set {
139-
dependency = newValue
145+
lock.withLock {
146+
dependency = newValue
147+
initialized = true
148+
}
140149
}
141150
}
142-
151+
143152
/// Unwraps the property wrapper granting access to the resolve/reset function.
144153
public var projectedValue: LazyInjected<T> {
145154
get { return self }
146155
mutating set { self = newValue }
147156
}
148-
157+
149158
/// Grants access to the internal Factory.
150159
public var factory: Factory<T> {
151160
thunk()
152161
}
153-
162+
154163
/// Allows the user to force a Factory resolution at their discretion.
155164
public mutating func resolve(reset options: FactoryResetOptions = .none) {
156-
let factory = thunk()
157-
factory.reset(options)
158-
dependency = factory()
159-
initialize = false
165+
lock.withLock {
166+
let factory = thunk()
167+
factory.reset(options)
168+
dependency = factory()
169+
initialized = true
170+
}
160171
}
161172

162173
/// Projected function returns resolved instance if it exists.
@@ -168,7 +179,7 @@ extension Injected: @unchecked Sendable where T: Sendable {}
168179
/// $myService.resolvedOrNil()?.cleanup()
169180
/// }
170181
public func resolvedOrNil() -> T? {
171-
dependency
182+
lock.withLock { initialized ? dependency : nil }
172183
}
173184

174185
}
@@ -198,50 +209,61 @@ extension LazyInjected: @unchecked Sendable where T: Sendable {}
198209

199210
private var thunk: () -> Factory<T>
200211
private weak var dependency: AnyObject?
201-
private var initialize = true
202-
212+
private var initialized = false
213+
private let lock: CrossPlatformLock
214+
203215
/// Initializes the property wrapper. The dependency isn't resolved until the wrapped value is accessed for the first time.
204216
/// - Parameter keyPath: KeyPath to a Factory on the default Container.
205217
public init(_ keyPath: KeyPath<Container, Factory<T>>) {
218+
self.lock = CrossPlatformLock()
206219
self.thunk = { Container.shared[keyPath: keyPath] }
207220
}
208-
221+
209222
/// Initializes the property wrapper. The dependency isn't resolved until the wrapped value is accessed for the first time.
210223
/// - Parameter keyPath: KeyPath to a Factory on the specified Container.
211224
public init<C:SharedContainer>(_ keyPath: KeyPath<C, Factory<T>>) {
225+
self.lock = CrossPlatformLock()
212226
self.thunk = { C.shared[keyPath: keyPath] }
213227
}
214-
228+
215229
/// Manages the wrapped dependency, which is resolved when this value is accessed for the first time.
216230
public var wrappedValue: T? {
217231
mutating get {
218-
if initialize {
219-
resolve()
232+
lock.withLock {
233+
if !initialized {
234+
dependency = thunk()() as AnyObject
235+
initialized = true
236+
}
237+
return dependency as? T
220238
}
221-
return dependency as? T
222239
}
223240
mutating set {
224-
dependency = newValue as AnyObject
241+
lock.withLock {
242+
dependency = newValue as AnyObject
243+
initialized = true
244+
}
225245
}
226246
}
227-
247+
228248
/// Unwraps the property wrapper granting access to the resolve/reset function.
229249
public var projectedValue: WeakLazyInjected<T> {
230250
get { return self }
231251
mutating set { self = newValue }
232252
}
233-
253+
234254
/// Grants access to the internal Factory.
235255
public var factory: Factory<T> {
236256
thunk()
237257
}
238-
258+
239259
/// Allows the user to force a Factory resolution at their discretion.
240260
public mutating func resolve(reset options: FactoryResetOptions = .none) {
241-
let factory = thunk()
242-
factory.reset(options)
243-
dependency = factory() as AnyObject
244-
initialize = false
261+
lock.withLock {
262+
let factory = thunk()
263+
factory.reset(options)
264+
dependency = factory() as AnyObject
265+
initialized = true
266+
}
245267
}
246268

247269
/// Projected function returns resolved instance if it exists.
@@ -253,7 +275,7 @@ extension LazyInjected: @unchecked Sendable where T: Sendable {}
253275
/// $myService.resolvedOrNil()?.cleanup()
254276
/// }
255277
public func resolvedOrNil() -> T? {
256-
dependency as? T
278+
lock.withLock { dependency as? T }
257279
}
258280

259281
}

Sources/FactoryKit/FactoryKit/Locking.swift

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,22 @@ internal final class RecursiveLock: NSLocking {
6262
}
6363

6464
/// Master variable spin lock
65-
nonisolated(unsafe) internal let globalVariableLock = SpinLock()
65+
internal let globalVariableLock = CrossPlatformLock()
6666

6767
#if os(macOS) || os(iOS) || os(watchOS)
68-
/// Custom spin lock
69-
internal final class SpinLock: NSLocking {
68+
/// Custom lock using os_unfair_lock on Apple platforms.
69+
internal final class CrossPlatformLock: NSLocking, @unchecked Sendable {
7070

7171
init() {
7272
oslock = UnsafeMutablePointer<os_unfair_lock>.allocate(capacity: 1)
7373
oslock.initialize(to: .init())
7474
}
7575

76+
deinit {
77+
oslock.deinitialize(count: 1)
78+
oslock.deallocate()
79+
}
80+
7681
@inlinable @inline(__always) func lock() {
7782
os_unfair_lock_lock(oslock)
7883
}
@@ -85,8 +90,8 @@ internal final class SpinLock: NSLocking {
8590

8691
}
8792
#else
88-
/// Custom spin lock compatible with Linux
89-
internal final class SpinLock: NSLocking {
93+
/// Custom lock compatible with Linux using pthread_mutex.
94+
internal final class CrossPlatformLock: NSLocking, @unchecked Sendable {
9095

9196
init() {
9297
mutex = UnsafeMutablePointer<pthread_mutex_t>.allocate(capacity: 1)
@@ -98,6 +103,11 @@ internal final class SpinLock: NSLocking {
98103
attributes.deallocate()
99104
}
100105

106+
deinit {
107+
pthread_mutex_destroy(mutex)
108+
mutex.deallocate()
109+
}
110+
101111
@inlinable @inline(__always) func lock() {
102112
pthread_mutex_lock(mutex)
103113
}
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
import XCTest
2+
@testable import FactoryKit
3+
4+
final class FactoryLazyInjectedLockingTests: XCTestCase, @unchecked Sendable {
5+
6+
override func setUp() {
7+
super.setUp()
8+
LazyLockingContainer.shared.reset()
9+
Scope.singleton.reset()
10+
}
11+
12+
// Verifies the core guarantee: even when many threads race on first access,
13+
// the factory closure is invoked exactly once.
14+
func testConcurrentFirstAccessInvokesFactoryExactlyOnce() {
15+
let counter = InvocationCounter()
16+
LazyLockingContainer.shared.service.register {
17+
counter.increment()
18+
return LazyLockingService()
19+
}
20+
21+
let holder = LazyInjectedLockingHolder()
22+
DispatchQueue.concurrentPerform(iterations: 50) { _ in
23+
_ = holder.service
24+
}
25+
26+
XCTAssertEqual(counter.value, 1, "@LazyInjected factory should be invoked exactly once under concurrent first access, got \(counter.value)")
27+
}
28+
29+
// Runs the exactly-once check multiple times to catch probabilistic races.
30+
func testConcurrentFirstAccessInvokesFactoryExactlyOnceRepeated() {
31+
for _ in 0..<20 {
32+
LazyLockingContainer.shared.reset()
33+
let counter = InvocationCounter()
34+
LazyLockingContainer.shared.service.register {
35+
counter.increment()
36+
return LazyLockingService()
37+
}
38+
39+
let holder = LazyInjectedLockingHolder()
40+
DispatchQueue.concurrentPerform(iterations: 50) { _ in
41+
_ = holder.service
42+
}
43+
44+
XCTAssertEqual(counter.value, 1, "Factory invocation count should be 1, got \(counter.value)")
45+
}
46+
}
47+
48+
// Same guarantee for @WeakLazyInjected.
49+
func testWeakLazyInjectedConcurrentFirstAccessInvokesFactoryExactlyOnce() {
50+
let counter = InvocationCounter()
51+
LazyLockingContainer.shared.service.register {
52+
counter.increment()
53+
return LazyLockingService()
54+
}
55+
56+
let holder = WeakLazyInjectedLockingHolder()
57+
// Keep strong references so the weak dependency isn't released mid-test.
58+
let retained = RetainBag<LazyLockingService>()
59+
DispatchQueue.concurrentPerform(iterations: 50) { _ in
60+
if let svc = holder.service {
61+
retained.append(svc)
62+
}
63+
}
64+
65+
XCTAssertEqual(counter.value, 1, "@WeakLazyInjected factory should be invoked exactly once under concurrent first access, got \(counter.value)")
66+
retained.discard()
67+
}
68+
69+
// resolvedOrNil() should return nil before first access and non-nil after.
70+
func testResolvedOrNilSemanticsBeforeAndAfterAccess() {
71+
let holder = LazyInjectedLockingHolder()
72+
XCTAssertNil(holder.resolvedOrNil, "resolvedOrNil() should be nil before first access")
73+
_ = holder.service
74+
XCTAssertNotNil(holder.resolvedOrNil, "resolvedOrNil() should be non-nil after first access")
75+
}
76+
77+
// resolve(reset:) should re-invoke the factory and result in a non-nil dependency.
78+
func testExplicitResolveUpdatesValue() {
79+
let holder = LazyInjectedLockingHolder()
80+
let first = holder.service
81+
holder.$service.resolve(reset: .none)
82+
let second = holder.resolvedOrNil
83+
XCTAssertNotNil(second)
84+
XCTAssertFalse(first === second, "resolve() should produce a new instance with a unique-scoped factory")
85+
}
86+
87+
}
88+
89+
// MARK: - Infrastructure
90+
91+
private final class InvocationCounter: @unchecked Sendable {
92+
private let lock = NSLock()
93+
private var count = 0
94+
func increment() { lock.withLock { count += 1 } }
95+
var value: Int { lock.withLock { count } }
96+
}
97+
98+
private final class RetainBag<T: AnyObject>: @unchecked Sendable {
99+
private let lock = NSLock()
100+
private var storage: [T] = []
101+
func append(_ element: T) { lock.withLock { storage.append(element) } }
102+
func discard() { lock.withLock { storage.removeAll() } }
103+
}
104+
105+
final class LazyLockingService: @unchecked Sendable {
106+
init() {}
107+
}
108+
109+
final class LazyLockingContainer: SharedContainer {
110+
static let shared = LazyLockingContainer()
111+
var service: Factory<LazyLockingService> { self { LazyLockingService() } }
112+
let manager = ContainerManager()
113+
}
114+
115+
final class LazyInjectedLockingHolder: @unchecked Sendable {
116+
@LazyInjected(\LazyLockingContainer.service) var service: LazyLockingService
117+
var resolvedOrNil: LazyLockingService? { $service.resolvedOrNil() }
118+
}
119+
120+
final class WeakLazyInjectedLockingHolder: @unchecked Sendable {
121+
@WeakLazyInjected(\LazyLockingContainer.service) var service: LazyLockingService?
122+
}

0 commit comments

Comments
 (0)