Hi! I think I have found a potential issue related to Async
and AsyncResult
in the code base. As Async
executes submitted completion handlers on its own private queue, there is a potential for data races when manipulating shared resources.
To reproduce
Consider the following test:
import XCTest
@testable import ConfigCat
class CrashAsyncTests: XCTestCase {
func testNotCrashing() {
let numberOfAttempts = 10
let expectation = self.expectation(description: "wait for all attempts to mutate dictionary")
expectation.assertForOverFulfill = true
expectation.expectedFulfillmentCount = numberOfAttempts
for i in 0..<numberOfAttempts {
var sharedResource = ["": ""]
let async = Async()
async.apply {
sharedResource["\(i)"] = "blargh"
expectation.fulfill()
}
async.complete()
}
wait(for: [expectation], timeout: .pi)
}
func testCrashing() {
let numberOfAttempts = 10
let expectation = self.expectation(description: "wait for all attempts to mutate dictionary")
expectation.assertForOverFulfill = true
expectation.expectedFulfillmentCount = numberOfAttempts
var sharedResource = ["": ""]
for i in 0..<numberOfAttempts {
let async = Async()
async.apply {
sharedResource["\(i)"] = "blargh"
expectation.fulfill()
}
async.complete()
}
wait(for: [expectation], timeout: .pi)
}
}
When running the test above with thread sanitizer enabled, one can indeed observe a data race:
![Skjermbilde 2022-07-14 kl 15 32 43](https://user-images.githubusercontent.com/2263015/178994510-5fc2849b-81c9-4a60-97db-08d1e70a791a.png)
In and of itself, one can stipulate that Async
inherently is not thread safe, which is not necessarily problematic. The issue arises when one is manipulating a shared resources via different instances of Async
or AsyncResult
.
One example of this, can be found in the way AutoPollingPolicy.writeConfigCache|readConfigCache
are called in closures passed to instances of AsyncResult
. See AutoPollingPolicy:59
:
timer.setEventHandler(handler: { [weak self] in
guard let `self` = self else {
return
}
if self.fetcher.isFetching() {
self.log.debug(message: "Config fetching is skipped because there is an ongoing fetch request")
return;
}
self.fetcher.getConfiguration()
.apply(completion: { response in
let cached = self.readConfigCache()
if let config = response.config, response.isFetched() && config.jsonString != cached.jsonString {
self.writeConfigCache(value: config) // <- writing to property on Async's private queue
self.onConfigChanged?()
}
if !self.initialized.getAndSet(new: true) {
self.initResult.complete()
}
})
})
and in AutoPollingPolicy:95
:
public override func getConfiguration() -> AsyncResult<Config> {
if self.initResult.completed {
return self.readCacheAsync()
}
return self.initResult.apply(completion: {
return self.readConfigCache() // <- reading from property on Async's private queue
})
}
While access to properties are enforced some places in the code base with Synced
, in the said examples the code is inherently not thread safe. It is perhaps unfortunate that a by-effect of Async
's implementation leads to dispatching code on otherwise inaccessible queues to code from the outside, such that synchronization is needed.
Another issue is that AsyncResult
itself is not thread safe. In AsyncResult:96
:
public func complete(result: Value) {
self.result = result // <- assignment on the caller's queue
super.complete()
}
we can see that self.result
is assigned the argument of complete(result:)
. As it is not private, the method will be called on the caller's queue. In AsyncResult:122
:
@discardableResult
public func apply(completion: @escaping (Value) -> Void) -> AsyncResult<Value> {
self.queue.async {
if self.state.get().isCompleted() {
guard let result = self.result else { // <- reading self.result on private queue
assert(false, "completion handlers executed on an incomplete AsyncResult")
return
}
completion(result)
} else {
self.completions.append({
guard let result = self.result else {
assert(false, "completion handlers executed on an incomplete AsyncResult")
return
}
completion(result)
})
}
}
return self
}
Thus, it is evident there is a potential for accessing self.result
on different queues.
Conversely, the property Async.state
is wrapped in a thread safe container / actor. However, since this is a private property, and it is seemingly only accessed on Async.queue
, it never had to be wrapped in synchronization code in the first place.