BoolVariation is not thread safe

[magnushakansson]

Is this a support request? No

Describe the bug When BoolVariation is called on multiple threads, there is a segv.

To reproduce I have modified the example program in https://github.com/launchdarkly/cpp-sdks/blob/main/examples/hello-cpp-server/main.cpp. Replace lines https://github.com/launchdarkly/cpp-sdks/blob/main/examples/hello-cpp-server/main.cpp#L58C1-L66C1 with:

     auto LoopGetFeatures = [&client] {
        while (true) {
            auto const context = ContextBuilder().Kind("user", "example-user-key").Name("Sandy").Build();
            bool const flag_value = client.BoolVariation(context, FEATURE_FLAG_KEY, false);
        }
    };

    std::thread t1(LoopGetFeatures);
    std::thread t2(LoopGetFeatures);
    std::thread t3(LoopGetFeatures);
    std::thread t4(LoopGetFeatures);

    t1.join();
    t2.join();
    t3.join();
    t4.join();

Make sure that FEATURE_FLAG_KEY is set in the backend you use.

Expected behavior The example program shall run without problem.

Logs Before the program crashes, you will see a number of

[LaunchDarkly] Invalid flag configuration detected in flag "(no parent)": prerequisite relationship to "GLOBAL_FLAG_TRUE" caused a circular reference; this is probably a temporary condition due to an incomplete update

SDK version 3.4.0

Language version, developer tools C++20, Clang 17.

OS/platform Debian 11

Additional context It seems like the two sets in https://github.com/launchdarkly/cpp-sdks/blob/main/libs/server-sdk/src/evaluation/detail/evaluation_stack.hpp#L56C1-L57C52 lack protection. When I add a mutex for them, the crash is avoided but the log line mentioned above is still there so something more seems to be needed.

[cwaldren-ld]

Hi @magnushakansson , you are correct that it is not thread-safe, and I apologize for having to discover it yourself rather than read it in documentation. That was an oversight.

We'd like to get this properly supported so that it's safe to call from multiple threads in parallel. This involves creating a new EvaluationStack for each call, rather than sharing one.

Would you mind checking out this branch and re-testing the scenario on your machine: cw/sc-235941/thread-safe-evaluation?

On my machine, it no longer crashes.

[magnushakansson]

Would you mind checking out this branch and re-testing the scenario on your machine: cw/sc-235941/thread-safe-evaluation?

On my machine, it no longer crashes.

@cwaldren-ld I can confirm that I cannot longer reproduce the problem. Thanks for the quick fix!

Any estimate when you can release this?

[cwaldren-ld]

Hi @magnushakansson, I'd like to add more multithread tests at other layers of the SDK. I'd like to complete this before Friday but can't promise anything.

If I haven't followed up feel free to ping me again on this issue.

[cwaldren-ld]

Hi @magnushakansson , I'm unable to prioritize getting this merged, but I'll give an update when I'm able.

[cwaldren-ld]

Hi @magnushakansson , this is fixed in v3.3.5. Thank you for your patience.

I will note I discovered some potential thread safety issues if you happen to be using the Redis integration that are not yet resolved.