feat: Thread safe isEnabled and getVariant

[kwasniew]

About the changes

Fix Thread Safety Issue in UnleashProxyClientSwift

Problem

The UnleashProxyClientSwift client was not thread-safe, which could lead to race conditions and crashes when:

• Multiple threads simultaneously called isEnabled() or getVariant()
• Context updates occurred while other operations were in progress
• Impression events were being posted while the client state was changing

A dedicated test case (UnleashThreadSafetyTest.swift) was created to reproduce these issues by simulating concurrent access from multiple threads.

Solution

The following changes were implemented to make the client thread-safe:

1. Made isEnabled() and getVariant() methods thread-safe by using synchronized access
2. Ensured that start() and updateContext() methods always execute on the main thread
3. Moved impression event posting to a background queue to avoid blocking the main thread

Testing

The fix was verified using a dedicated thread safety test that simulates heavy concurrent access to the client from multiple threads, including:

• Concurrent calls to isEnabled()
• Concurrent calls to getVariant()
• Context updates while other operations are in progress

The test runs multiple iterations to ensure the fix is robust and reliable.

Important files

Discussion points

[kwasniew]

this test will fail with the previous version of the client

[daveleek]

Ok, so in essence we're making sure it runs on the main thread and not just ignoring it if it isn't. Good. Then we shouldn't be breaking things

[daveleek]

Ok looks good. Re running start/updateContext on the main thread, is that the final decision or should we consider running them on either a separate thread or through a separate synchronization queue?

[benjamin-es-hall]

I'm not 100% sure if the checking if isMainThread is necessary.

If we weren't on the main thread we dispatch it to the main thread (else). If we are though, I imagine dispatching to main just tells it to continue. Maybe there is some performance improvement here but I imagine it is miner.

It may just be simpler to always dispatch to main thread. The test case should prove if this is fine by removing the if check.

[kwasniew]

Thanks. Will check this one tomorrow

[kwasniew]

I think we can't skip the first branch as it would break the synchronous behavior that some tests and client code expect.

[benjamin-es-hall]

Would this not make the unit test last at least 10seconds, which is pretty long for a unit test. Which resources require proper releasing that requires this length time interval?

Given I don't think this is a major issue since testing something valid, but does put a relatively high lower limit on any unit testing suites

[kwasniew]

This test is slow even without this line. TBH I added this test when I was reproducing the bug but I'd prefer not to run it with the main unit test suite. It was just convenient for me to use it for diagnostics and wanted to share it with reviewers. I can try extracting the essence of this test into a much faster test or separate it from the unit tests suite. Resources = timers, network connections, threads etc.

[anna-gn]

Hi, I don't think a concurrent queue will help with the data race issue, but maybe there was a different purpose to it?
Here is a test I used to simulate the race with 2.2.0 (it crashes on 2.3.0 as well):

func testThreading() {
    for _ in 0..<10000 {
        client.updateContext(context: ["userId": "1"], properties: ["isAuthenticated": "true"]) { _ in }
        DispatchQueue.global(qos: .userInitiated).async { [weak self] in
            _ = self?.client.isEnabled(name: "bool_parameter")
        }
        DispatchQueue.global(qos: .utility).async { [weak self] in
            self?.client.start(bootstrap: .toggles([]), false) { _ in }
        }
        DispatchQueue.global(qos: .default).async { [weak self] in
            _ = self?.client.getVariant(name: "string_parameter")
        }
    }
}
    ```

[anna-gn]

In this PR, only isEnabled and getVariant use the queue. To protect shared state, other public APIs would need some kind of synchronization too. Wondering if you considered using locks instead of the queue?