fix: Evaluate should not share EvaluationStack between calls #374
Conversation
cwaldren-ld
changed the title
cwaldren-ld
changed the title
cwaldren-ld
force-pushed
the
cw/sc-235941/thread-safe-evaluation
branch
2 times, most recently
from
da3fcc0 to
820e910
Compare
|
There are additional thread-safety concerns I've discovered while testing this. Specifically, although we may be able to make the evaluator thread safe, we need to ensure the data system implementations are thread safe as well. Specific notes:
Still, this PR is an incremental improvement and can be merged independently. |
cwaldren-ld
force-pushed
the
cw/sc-235941/thread-safe-evaluation
branch
from
820e910 to
410339a
Compare
| @@ -246,3 +249,30 @@ TEST_F(EvaluatorTests, FlagWithExperimentTargetingMissingContext) { | |||
| ASSERT_EQ(*detail, Value(false)); | |||
| ASSERT_EQ(detail.Reason(), EvaluationReason::Fallthrough(false)); | |||
| } | |||
|
|
|||
| TEST_F(EvaluatorTests, ThreadSafeEvaluation) { | |||
There was a problem hiding this comment.
This type of test is what I've done in the past, and I'm not really sure of a better way to do it.
Basically spin up a bunch of threads and see if ASAN is triggered.
| * @param event_scope The event scope used for recording prerequisite | ||
| * events. | ||
| */ | ||
| [[nodiscard]] EvaluationDetail<Value> Evaluate( | ||
| data_model::Flag const& flag, | ||
| Context const& context, | ||
| EvaluationStack& stack, |
There was a problem hiding this comment.
Is there a reason the public API needs to take a stack versus making one. Or are we re-using the public version internal to the evaluator as well?
There was a problem hiding this comment.
We also may want to consider a more generic EvaluationState that contains the evaluation stack.
Eventually you have big segment status and big segment membership that persist the duration of an evaluation.
There was a problem hiding this comment.
Yes, we are using this in AllFlags. So we setup the stack object once, then run all the evaluations on it.
I think I did that because it felt bad to be constructing the stack over and over for each flag - but I didn't have any concrete data, so I'd be fine pushing it back into the Evaluator.
It'd be safer that way too, if the stack somehow got un-balanced by a failed evaluation.
cwaldren-ld
force-pushed
the
cw/sc-235941/thread-safe-evaluation
branch
from
410a8dd to
b39c9af
Compare
🤖 I have created a release *beep* *boop* --- <details><summary>launchdarkly-cpp-server: 3.3.5</summary> ## [3.3.5](launchdarkly-cpp-server-v3.3.4...launchdarkly-cpp-server-v3.3.5) (2024-04-04) ### Bug Fixes * Evaluate should not share EvaluationStack between calls ([#374](#374)) ([7fd64ef](7fd64ef)) </details> <details><summary>launchdarkly-cpp-server-redis-source: 2.1.5</summary> ## [2.1.5](launchdarkly-cpp-server-redis-source-v2.1.4...launchdarkly-cpp-server-redis-source-v2.1.5) (2024-04-04) ### Dependencies * The following workspace dependencies were updated * dependencies * launchdarkly-cpp-server bumped from 3.3.4 to 3.3.5 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Addresses #373.