fix: Evaluate should not share EvaluationStack between calls (#374)

Addresses #373.

Author: cwaldren-ld

libs/server-sdk/src/CMakeLists.txt

@@ -60,7 +60,7 @@ target_sources(${LIBNAME}
         evaluation/bucketing.cpp
         evaluation/operators.cpp
         evaluation/evaluation_error.cpp
-        evaluation/detail/evaluation_stack.cpp
+        evaluation/evaluation_stack.cpp
         evaluation/detail/semver_operations.cpp
         evaluation/detail/timestamp_operations.cpp
         events/event_factory.cpp

libs/server-sdk/src/client_impl.cpp

@@ -4,6 +4,7 @@
 #include "data_systems/background_sync/background_sync_system.hpp"
 #include "data_systems/lazy_load/lazy_load_system.hpp"
 #include "data_systems/offline.hpp"
+#include "evaluation/evaluation_stack.hpp"
 
 #include "data_interfaces/system/idata_system.hpp"
 

libs/server-sdk/src/evaluation/detail/evaluation_stack.cpp renamed to libs/server-sdk/src/evaluation/evaluation_stack.cpp

@@ -1,6 +1,6 @@
 #include "evaluation_stack.hpp"
 
-namespace launchdarkly::server_side::evaluation::detail {
+namespace launchdarkly::server_side::evaluation {
 
 Guard::Guard(std::unordered_set<std::string>& set, std::string key)
     : set_(set), key_(std::move(key)) {
@@ -27,4 +27,4 @@ std::optional<Guard> EvaluationStack::NoticeSegment(std::string segment_key) {
     return std::make_optional<Guard>(segments_seen_, std::move(segment_key));
 }
 
-}  // namespace launchdarkly::server_side::evaluation::detail
+}  // namespace launchdarkly::server_side::evaluation

libs/server-sdk/src/evaluation/detail/evaluation_stack.hpp renamed to libs/server-sdk/src/evaluation/evaluation_stack.hpp

@@ -4,7 +4,7 @@
 #include <string>
 #include <unordered_set>
 
-namespace launchdarkly::server_side::evaluation::detail {
+namespace launchdarkly::server_side::evaluation {
 
 /**
  * Guard is an object used to track that a segment or flag key has been noticed.
@@ -57,4 +57,4 @@ class EvaluationStack {
     std::unordered_set<std::string> segments_seen_;
 };
 
-}  // namespace launchdarkly::server_side::evaluation::detail
+}  // namespace launchdarkly::server_side::evaluation

libs/server-sdk/src/evaluation/evaluator.cpp

@@ -1,4 +1,5 @@
 #include "evaluator.hpp"
+#include "bucketing.hpp"
 #include "rules.hpp"
 
 #include <boost/core/ignore_unused.hpp>
@@ -20,7 +21,7 @@ std::optional<std::size_t> TargetMatchVariation(
     Flag::Target const& target);
 
 Evaluator::Evaluator(Logger& logger, data_interfaces::IStore const& source)
-    : logger_(logger), source_(source), stack_() {}
+    : logger_(logger), source_(source) {}
 
 EvaluationDetail<Value> Evaluator::Evaluate(
     data_model::Flag const& flag,
@@ -32,15 +33,17 @@ EvaluationDetail<Value> Evaluator::Evaluate(
     Flag const& flag,
     launchdarkly::Context const& context,
     EventScope const& event_scope) {
-    return Evaluate(std::nullopt, flag, context, event_scope);
+    EvaluationStack stack;
+    return Evaluate(std::nullopt, flag, context, stack, event_scope);
 }
 
 EvaluationDetail<Value> Evaluator::Evaluate(
     std::optional<std::string> parent_key,
     Flag const& flag,
     launchdarkly::Context const& context,
+    EvaluationStack& stack,
     EventScope const& event_scope) {
-    if (auto guard = stack_.NoticePrerequisite(flag.key)) {
+    if (auto guard = stack.NoticePrerequisite(flag.key)) {
         if (!flag.on) {
             return OffValue(flag, EvaluationReason::Off());
         }
@@ -63,8 +66,8 @@ EvaluationDetail<Value> Evaluator::Evaluate(
             }
 
             // Recursive call; cycles are detected by the guard.
-            EvaluationDetail<Value> detailed_evaluation =
-                Evaluate(flag.key, *descriptor.item, context, event_scope);
+            EvaluationDetail<Value> detailed_evaluation = Evaluate(
+                flag.key, *descriptor.item, context, stack, event_scope);
 
             if (detailed_evaluation.IsError()) {
                 return detailed_evaluation;
@@ -106,7 +109,7 @@ EvaluationDetail<Value> Evaluator::Evaluate(
         auto const& rule = flag.rules[rule_index];
 
         tl::expected<bool, Error> rule_match =
-            Match(rule, context, source_, stack_);
+            Match(rule, context, source_, stack);
 
         if (!rule_match) {
             LogError(flag.key, rule_match.error());

libs/server-sdk/src/evaluation/evaluator.hpp

@@ -6,27 +6,30 @@
 #include <launchdarkly/logging/logger.hpp>
 #include <launchdarkly/value.hpp>
 
-#include "bucketing.hpp"
-#include "detail/evaluation_stack.hpp"
 #include "evaluation_error.hpp"
+#include "evaluation_stack.hpp"
 
 #include "../data_interfaces/store/istore.hpp"
 #include "../events/event_scope.hpp"
 
-#include <tl/expected.hpp>
-
 namespace launchdarkly::server_side::evaluation {
 
 class Evaluator {
    public:
+    /**
+     * Constructs a new Evaluator. Since the Evaluator may be used by multiple
+     * threads in parallel, the given logger and IStore must be thread safe.
+     * @param logger A logger for recording errors or warnings.
+     * @param source The flag/segment source.
+     */
     Evaluator(Logger& logger, data_interfaces::IStore const& source);
 
     /**
      * Evaluates a flag for a given context.
-     * Warning: not thread safe.
      *
      * @param flag The flag to evaluate.
      * @param context The context to evaluate the flag against.
+     * @param stack The evaluation stack used for detecting circular references.
      * @param event_scope The event scope used for recording prerequisite
      * events.
      */
@@ -37,7 +40,7 @@ class Evaluator {
 
     /**
      * Evaluates a flag for a given context. Does not record prerequisite
-     * events. Warning: not thread safe.
+     * events.
      *
      * @param flag The flag to evaluate.
      * @param context The context to evaluate the flag against.
@@ -50,6 +53,7 @@ class Evaluator {
         std::optional<std::string> parent_key,
         data_model::Flag const& flag,
         Context const& context,
+        EvaluationStack& stack,
         EventScope const& event_scope);
 
     [[nodiscard]] EvaluationDetail<Value> FlagVariation(
@@ -65,6 +69,5 @@ class Evaluator {
 
     Logger& logger_;
     data_interfaces::IStore const& source_;
-    detail::EvaluationStack stack_;
 };
 }  // namespace launchdarkly::server_side::evaluation

libs/server-sdk/src/evaluation/rules.cpp

@@ -16,7 +16,7 @@ bool MaybeNegate(Clause const& clause, bool value) {
 tl::expected<bool, Error> Match(Flag::Rule const& rule,
                                 launchdarkly::Context const& context,
                                 data_interfaces::IStore const& store,
-                                detail::EvaluationStack& stack) {
+                                EvaluationStack& stack) {
     for (Clause const& clause : rule.clauses) {
         tl::expected<bool, Error> result = Match(clause, context, store, stack);
         if (!result) {
@@ -32,7 +32,7 @@ tl::expected<bool, Error> Match(Flag::Rule const& rule,
 tl::expected<bool, Error> Match(Segment::Rule const& rule,
                                 Context const& context,
                                 data_interfaces::IStore const& store,
-                                detail::EvaluationStack& stack,
+                                EvaluationStack& stack,
                                 std::string const& key,
                                 std::string const& salt) {
     for (Clause const& clause : rule.clauses) {
@@ -62,7 +62,7 @@ tl::expected<bool, Error> Match(Segment::Rule const& rule,
 tl::expected<bool, Error> Match(Clause const& clause,
                                 launchdarkly::Context const& context,
                                 data_interfaces::IStore const& store,
-                                detail::EvaluationStack& stack) {
+                                EvaluationStack& stack) {
     if (clause.op == Clause::Op::kSegmentMatch) {
         return MatchSegment(clause, context, store, stack);
     }
@@ -72,7 +72,7 @@ tl::expected<bool, Error> Match(Clause const& clause,
 tl::expected<bool, Error> MatchSegment(Clause const& clause,
                                        launchdarkly::Context const& context,
                                        data_interfaces::IStore const& store,
-                                       detail::EvaluationStack& stack) {
+                                       EvaluationStack& stack) {
     for (Value const& value : clause.values) {
         // A segment key represented as a Value is a string; non-strings are
         // ignored.
@@ -154,7 +154,7 @@ tl::expected<bool, Error> MatchNonSegment(
 tl::expected<bool, Error> Contains(Segment const& segment,
                                    Context const& context,
                                    data_interfaces::IStore const& store,
-                                   detail::EvaluationStack& stack) {
+                                   EvaluationStack& stack) {
     auto guard = stack.NoticeSegment(segment.key);
     if (!guard) {
         return tl::make_unexpected(Error::CyclicSegmentReference(segment.key));

libs/server-sdk/src/evaluation/rules.hpp

@@ -1,8 +1,8 @@
 #pragma once
 
 #include "../data_interfaces/store/istore.hpp"
-#include "detail/evaluation_stack.hpp"
 #include "evaluation_error.hpp"
+#include "evaluation_stack.hpp"
 
 #include <launchdarkly/context.hpp>
 #include <launchdarkly/data_model/flag.hpp>
@@ -18,26 +18,26 @@ namespace launchdarkly::server_side::evaluation {
     data_model::Flag::Rule const&,
     Context const&,
     data_interfaces::IStore const& store,
-    detail::EvaluationStack& stack);
+    EvaluationStack& stack);
 
 [[nodiscard]] tl::expected<bool, Error> Match(data_model::Clause const&,
                                               Context const&,
                                               data_interfaces::IStore const&,
-                                              detail::EvaluationStack&);
+                                              EvaluationStack&);
 
 [[nodiscard]] tl::expected<bool, Error> Match(
     data_model::Segment::Rule const& rule,
     Context const& context,
     data_interfaces::IStore const& store,
-    detail::EvaluationStack& stack,
+    EvaluationStack& stack,
     std::string const& key,
     std::string const& salt);
 
 [[nodiscard]] tl::expected<bool, Error> MatchSegment(
     data_model::Clause const&,
     Context const&,
     data_interfaces::IStore const&,
-    detail::EvaluationStack& stack);
+    EvaluationStack& stack);
 
 [[nodiscard]] tl::expected<bool, Error> MatchNonSegment(
     data_model::Clause const&,
@@ -47,7 +47,7 @@ namespace launchdarkly::server_side::evaluation {
     data_model::Segment const&,
     Context const&,
     data_interfaces::IStore const& store,
-    detail::EvaluationStack& stack);
+    EvaluationStack& stack);
 
 [[nodiscard]] bool MaybeNegate(data_model::Clause const& clause, bool value);
 

libs/server-sdk/tests/evaluation_stack_test.cpp

@@ -1,8 +1,8 @@
 #include <gtest/gtest.h>
 
-#include "evaluation/detail/evaluation_stack.hpp"
+#include "evaluation/evaluation_stack.hpp"
 
-using namespace launchdarkly::server_side::evaluation::detail;
+using namespace launchdarkly::server_side::evaluation;
 
 TEST(EvalStackTests, SegmentIsNoticed) {
     EvaluationStack stack;

libs/server-sdk/tests/evaluator_tests.cpp

@@ -8,6 +8,9 @@
 
 #include <gtest/gtest.h>
 
+#include <random>
+#include <thread>
+
 using namespace launchdarkly;
 using namespace launchdarkly::server_side;
 
@@ -246,3 +249,30 @@ TEST_F(EvaluatorTests, FlagWithExperimentTargetingMissingContext) {
     ASSERT_EQ(*detail, Value(false));
     ASSERT_EQ(detail.Reason(), EvaluationReason::Fallthrough(false));
 }
+
+TEST_F(EvaluatorTests, ThreadSafeEvaluation) {
+    auto flag = store_->GetFlag("flagWithTarget")->item.value();
+
+    constexpr std::size_t kNumThreads = 20;
+
+    std::vector<std::thread> threads;
+
+    for (std::size_t i = 0; i < kNumThreads; i++) {
+        constexpr std::size_t kNumIterations = 1000;
+        threads.emplace_back([this, &flag, kNumIterations] {
+            std::mt19937 generator(
+                std::chrono::system_clock::now().time_since_epoch().count());
+            std::uniform_int_distribution distribution(1, 3);
+            for (std::size_t j = 0; j < kNumIterations; j++) {
+                auto user_a = ContextBuilder().Kind("user", "userKeyA").Build();
+                auto _ = eval_.Evaluate(flag, user_a);
+                std::this_thread::sleep_for(
+                    std::chrono::milliseconds(distribution(generator)));
+            }
+        });
+    }
+
+    for (auto& t : threads) {
+        t.join();
+    }
+}

libs/server-sdk/tests/rule_tests.cpp

@@ -36,7 +36,7 @@ int const AllOperatorsTest::DATE_MS_NEGATIVE = -10000;
 const std::string AllOperatorsTest::INVALID_DATE = "hey what's this?";
 
 TEST_P(AllOperatorsTest, Matches) {
-    using namespace launchdarkly::server_side::evaluation::detail;
+    using namespace launchdarkly::server_side::evaluation;
     using namespace launchdarkly;
 
     auto const& param = GetParam();