issue when multiple threads accessing the registeredCalls dictionary

[vigdora]

when multiple threads are trying to access mock.registeredCalls it crashes.
any suggestion to a thread safe solution?

[danielsaidi]

Hi @vigdora

I haven't run into this, do you use multithreading in your tests?

[vigdora]

Hi @danielsaidi
when I run tests using XCTestCase, I see that multiple threads are running.
upon executing the next lines inside registerCall, it crashes:

let calls = mock.registeredCalls[ref.id] ?? [] 
mock.registeredCalls[ref.id] = calls + [call]

when I wrapped this code with serialQueue, it stopped crashing:

serialQueue.sync {
          let calls = mock.registeredCalls[ref.id] ?? []
          mock.registeredCalls[ref.id] = calls + [call]
      }

do you run unitests on one thread only?
tbh, I am not sure how to force XCTestCase to run only on one thread,
and even so, I suspect it might impact Performance.

[danielsaidi]

I usually don't change the default Xcode test setup, and assumed that running tests in parallel would still use isolated processes. So if you see this crash in the default setup, some form of thread safety is probably needed.

[vigdora]

@danielsaidi thanks for the quick reply.
it doesn"t happen when running tests in parallel. it actually happen upon running singular test suite. (with multiple tests inside it)
for example:

func fetchData() async throws {
   async let dataX = try await oneRepo.fetchX()
   async let dataY = await oneRepo.fetchY()
       
        let _: [Any] = try await [dataX, dataY ]
}

now lets assume oneRepo is mocked,
upon running the test, I have noticed the fetchX, and fetchY are running on different threads -
thats where the crash happening.
ofcourse I have simplified the code for the sake of example (:

[danielsaidi]

Thank you, that's great information 👍

Async functions run on different threads, so that is most definitely the problem. Perhaps it would be worth making the mock a main actor?

[vigdora]

yee I think thats a possible solution ( I will make sure it works in the meantime...)
but do you think it worth achieving an allistic solution, such that developers wont need to worry about it when they create a mock?
it took me some time to investigate (;
but anyWay, @mainactor might work here, as you suggested
and regardless, your package is really nice solution over all

[danielsaidi]

I most definitively want to add such a solution within the library itself, so devs don't have to do anything.

I only wish I can find a way to reproduce the problem.

[vigdora]

@danielsaidi
so I tried adding @mainactor on the mocked class and indeed the mocked methods now run only on the main thread.
but it didn't solve the issue, since inside async registerCall function I still see multiple thread running. (probably calling to call function is changing the thread as well)
you can try to reproduce the issue by creating something similar to the example I showed before,
and than run the test repeatedly around 100-200 times.
if it still won't reproduce I will create a branch for you with the issue later this week - let me know

[danielsaidi]

@vigdora

Thank you for continuing to look at this! I am unfortunately very busy with a few other projects, so I will not have time to look at this problem right now.

If you find a way to solve it, I'd be happy to merge it and release a new version. 👍

[tboogh]

We had similar issues with this so in our fork we use a semaphore to guard the access, it's not the most elegant solution but as a triage it works. https://github.com/BookBeat/MockingKit/tree/thread-safe

[danielsaidi]

@vigdora and @tboogh I haven't looked into this in a while, since I haven't used the library for some time. I enabled strict concurrency without getting any warnings whatsoever, which was a bit surprising to me, given that we may have this threading issue.