Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stuaustin opened this issue Nov 21, 2023 · 3 comments

Comments

@stuaustin
Copy link

stuaustin commented Nov 21, 2023

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:

  • Isolate the mocked function to an actor, e.g. MainActor. That way the mock's logic always runs on the same thread.
  • Isolate the history properties to an actor.
  • Isolate the entire class to an actor.
  • Use an actor instead of a class.

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)

@stuaustin stuaustin changed the title Async functions are unsafe in generated mocks, and can crash if used from multiple threads simulatenously Async functions are unsafe in generated mocks, and can crash if used from multiple threads simultaneously Nov 28, 2023
@bielikb
Copy link

bielikb commented Feb 19, 2024

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
Copy link

bielikb commented Apr 19, 2024

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

@sidepelican
Copy link
Collaborator

sidepelican commented Nov 6, 2024

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(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<Value>: @unchecked Sendable {
    var value: Value
    init<each T>(_ value: repeat each T) where Value == (repeat each T) {
        self.value = (repeat each value)
    }
}

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants