uber / mockolo

Efficient Mock Generator for Swift
Apache License 2.0
824 stars 87 forks source link

Async functions are unsafe in generated mocks, and can crash if used from multiple threads simultaneously #249

Open stuaustin opened 12 months ago

stuaustin commented 12 months ago

What did you do?

Created a mock for a protocol with an async function, e.g.

/// @mockable
protocol APIClient {
    func fetchData(_ id: AnyObject) async -> Int
}

This is the generated mock for the example code above:

class APIClientMock: APIClient {
    init() { }

    private(set) var fetchDataCallCount = 0
    var fetchDataArgValues = [AnyObject]()
    var fetchDataHandler: ((AnyObject) async -> (Int))?
    func fetchData(_ id: AnyObject) async -> Int {
        fetchDataCallCount += 1
        fetchDataArgValues.append(id)
        if let fetchDataHandler = fetchDataHandler {
            return await fetchDataHandler(id)
        }
        return 0
    }
}

What did you expect to happen?

The mock should be thread safe, or at least there should be a way to generate the mock in a thread safe manor. Some possibilities:

What happened instead?

The mock is not thread safe.

If you run the following code, the mock will crash with an EXC_BAD_ACCESS error on the fetchDataArgValues.append(id) line above.

        let mockAPI = APIClientMock()
        await withTaskGroup(of: Int.self, body: { taskGroup in
            for _ in 0..<1_000 {
                taskGroup.addTask {
                    await mockAPI.fetchData(NSObject())
                }
            }
            await taskGroup.waitForAll()
        })

This is because the fetchData function and fetchDataArgValues property are not actor isolated, and since the fetchData function is async it can be run on any thread. When multiple threads simultaneously try to read/write fetchDataArgValues this is unsafe, and the resultant EXC_BAD_ACCESS is caused by this.

Mockolo Environment

Mockolo Version: 2.0.1 Dependency Manager: mint Xcode Version: 15.0 Swift Version: Swift 5.9 (5.9.0.128.108)

bielikb commented 9 months ago

Hey Uber team,

our unit test suite faces reliability issues due to this issue.

What would be the best way to move this issue forward?

Best, Boris

bielikb commented 7 months ago

This PR might address the above issue, but I haven't verified it yet.

sidepelican commented 1 week ago

Draft:

/// @mockable
protocol APIClient: Sendable {
    func fetchData(_ id: AnyObject) async -> Int
}
final class APIClientMock: APIClient {
    init() { }

    private let fetchDataState = MockoloMutex(MockoloHandlerState<AnyObject, @Sendable (AnyObject) async -> Int>())
    var fetchDataCallCount: Int {
        return fetchDataState.withLock(\.callCount)
    }
    var fetchDataArgValues: [AnyObject] {
        return fetchDataState.withLock(\.argValues).map(\.value)
    }
    var fetchDataHandler: (@Sendable (AnyObject) async -> (Int))? {
        get { fetchDataState.withLock(\.handler) }
        set { fetchDataState.withLock { $0.handler = newValue } }
    }
    func fetchData(_ id: AnyObject) async -> Int {
        warnIfNotSendable(id)
        let handler = fetchDataState.withLock { state in
            state.callCount += 1
            state.argValues.append(.init(value: id))
            return state.handler
        }
        if let handler {
            return await handler(id)
        }
        return 0
    }
}

func warnIfNotSendable<each T>(function: String = #function, _: repeat each T) {
    print("At \(function), the captured arguments are not Sendable, it is not concurrency-safe.")
}

func warnIfNotSendable<each T: Sendable>(function: String = #function, _: repeat each T) {
}

import Foundation

/// Will be replaced to `Synchronization.Mutex` in future.
final class MockoloMutex<Value>: @unchecked Sendable {
    private let lock = NSLock()
    private var value: Value
    init(_ initialValue: Value) {
        self.value = initialValue
    }
#if compiler(>=6.0)
    func withLock<Result, E: Error>(_ body: (inout sending Value) throws(E) -> Result) throws(E) -> sending Result {
        lock.lock()
        defer { lock.unlock() }
        return try body(&value)
    }
#else
    func withLock<Result>(_ body: (inout Value) throws -> Result) rethrows -> Result {
        lock.lock()
        defer { lock.unlock() }
        return try body(&value)
    }
#endif
}

struct MockoloUnsafeTransfer<T>: @unchecked Sendable {
    var value: T
}

struct MockoloHandlerState<Arg, Handler> {
    var argValues: [MockoloUnsafeTransfer<Arg>] = []
    var handler: Handler? = nil
    var callCount: Int = 0
}