Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions openfeature/client_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,15 @@ template <typename ResolutionDetailsType, typename ValueType,
std::unique_ptr<ResolutionDetailsType> ClientAPI::EvaluateFlag(
ValueType default_value, const std::optional<EvaluationContext>& ctx,
ProviderCallable provider_call) {
ProviderStatus status = GetProviderStatus();
std::shared_ptr<FeatureProviderStatusManager> manager =
provider_repository_.GetFeatureProviderStatusManager(domain_);
if (!manager) {
return std::make_unique<ResolutionDetailsType>(
default_value, Reason::kError, std::nullopt, FlagMetadata(),
ErrorCode::kGeneral, "Provider status manager not found for domain");
}

ProviderStatus status = manager->GetStatus();
Comment thread
NeaguGeorgiana23 marked this conversation as resolved.
if (status == ProviderStatus::kNotReady) {
return std::make_unique<ResolutionDetailsType>(
default_value, Reason::kError, std::nullopt, FlagMetadata(),
Expand All @@ -125,8 +133,7 @@ std::unique_ptr<ResolutionDetailsType> ClientAPI::EvaluateFlag(
ErrorCode::kProviderFatal, "Provider is in fatal error state");
}

std::shared_ptr<FeatureProvider> provider =
provider_repository_.GetProvider(domain_);
std::shared_ptr<FeatureProvider> provider = manager->GetProvider();
if (!provider) {
return std::make_unique<ResolutionDetailsType>(
default_value, Reason::kError, std::nullopt, FlagMetadata(),
Expand Down
86 changes: 86 additions & 0 deletions test/client_api_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>

#include <atomic>
#include <chrono>
#include <future>
#include <memory>
#include <string>
#include <thread>

#include "absl/status/status.h"
#include "mocks/mock_feature_provider.h"
Expand Down Expand Up @@ -381,3 +385,85 @@

EXPECT_TRUE(client.GetBooleanValue("flag", false));
}

TEST_F(ClientAPITest, ParallelProviderSwapRaceCondition) {
std::string domain = "race-domain";
ClientAPI client(repo_, domain);
std::atomic<bool> evaluate{true};
std::atomic<bool> running{true};

// Continuously evaluate flags when allowed.
std::thread evaluation_thread([&]() {
while (running) {
if (evaluate) {
client.GetBooleanValue("flag", false);
} else {
std::this_thread::sleep_for(std::chrono::microseconds(10));

Check failure on line 401 in test/client_api_test.cpp

View workflow job for this annotation

GitHub Actions / lint

test/client_api_test.cpp:401:63 [readability-magic-numbers]

10 is a magic number; consider replacing it with a named constant
}
}
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Replace sleep-based race coordination with an explicit handshake.

Line 439 and Line 442 rely on scheduler timing. This can pass without any evaluation occurring while the provider is kNotReady, or fail if an in-flight evaluation crosses proceed_init->set_value() and calls not_ready_provider after it becomes ready. Track at least one evaluation during the blocked-init window, then drain in-flight evaluations before unblocking init.

Also applies to: 439-445

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/client_api_test.cpp` around lines 395 - 404, The race coordination in
the evaluation thread/test is still timing-based and can miss the blocked-init
window or let an in-flight call slip past unblocking. Update the test around the
evaluation_thread and the blocked-init handshake to explicitly record that at
least one GetBooleanValue("flag", false) evaluation occurred while the provider
was kNotReady, then wait for all in-flight evaluations to drain before calling
proceed_init->set_value(). Use the existing synchronization points in the test
fixture instead of std::this_thread::sleep_for to make the not_ready_provider
path deterministic.


// Continuously swap providers.
std::thread swap_thread([&]() {
int iterations = 50;

Check failure on line 408 in test/client_api_test.cpp

View workflow job for this annotation

GitHub Actions / lint

test/client_api_test.cpp:408:22 [readability-magic-numbers]

50 is a magic number; consider replacing it with a named constant
for (int i = 0; i < iterations && running; ++i) {
auto not_ready_provider =
std::make_shared<StrictMock<MockFeatureProvider>>();

auto init_called = std::make_shared<std::promise<void>>();
auto proceed_init = std::make_shared<std::promise<void>>();
auto init_finished = std::make_shared<std::promise<void>>();
std::shared_future<void> proceed_future =
proceed_init->get_future().share();

EXPECT_CALL(*not_ready_provider, Init(_))
.WillOnce(
testing::Invoke([init_called, proceed_future, init_finished](
const EvaluationContext&) -> absl::Status {
init_called->set_value();
proceed_future.wait();
init_finished->set_value();
return absl::OkStatus();
}));

EXPECT_CALL(*not_ready_provider, GetBooleanEvaluation(_, _, _)).Times(0);
EXPECT_CALL(*not_ready_provider, Shutdown())
.Times(testing::AtMost(1))
.WillOnce(Return(absl::OkStatus()));

repo_.SetProvider(domain, not_ready_provider,
EvaluationContext::Builder().build(), false);

init_called->get_future().wait();

std::this_thread::sleep_for(std::chrono::microseconds(100));

Check failure on line 439 in test/client_api_test.cpp

View workflow job for this annotation

GitHub Actions / lint

test/client_api_test.cpp:439:61 [readability-magic-numbers]

100 is a magic number; consider replacing it with a named constant

evaluate = false;
std::this_thread::sleep_for(std::chrono::microseconds(10));

Check failure on line 442 in test/client_api_test.cpp

View workflow job for this annotation

GitHub Actions / lint

test/client_api_test.cpp:442:61 [readability-magic-numbers]

10 is a magic number; consider replacing it with a named constant

proceed_init->set_value();
init_finished->get_future().wait();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound the future waits to avoid hanging the test suite.

If SetProvider stops invoking Init, or Init never reaches init_finished, these waits block forever and evaluation_thread keeps running. Prefer wait_for with a timeout and cleanup path that clears running and unblocks any pending promise.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/client_api_test.cpp` around lines 437 - 445, The test flow in the
SetProvider/Init handshake can hang forever because
init_called->get_future().wait() and init_finished->get_future().wait() have no
timeout. Update the client_api_test.cpp logic around SetProvider, Init, and
evaluation_thread to use wait_for with a bounded timeout, and add a cleanup path
that clears running and releases any pending promise if the timeout is hit. Make
sure the fix still preserves the current synchronization intent while preventing
the test suite from blocking indefinitely.


auto ready_provider = std::make_shared<NiceMock<MockFeatureProvider>>();
ON_CALL(*ready_provider, Init(_)).WillByDefault(Return(absl::OkStatus()));
ON_CALL(*ready_provider, GetBooleanEvaluation(_, _, _))
.WillByDefault(testing::Invoke(
[](std::string_view, bool, const EvaluationContext&)
-> absl::StatusOr<std::unique_ptr<BoolResolutionDetails>> {
return std::make_unique<BoolResolutionDetails>(
true, Reason::kTargetingMatch, std::nullopt,
FlagMetadata());
}));

repo_.SetProvider(domain, ready_provider,
EvaluationContext::Builder().build(), true);
evaluate = true;
std::this_thread::sleep_for(std::chrono::microseconds(100));

Check failure on line 461 in test/client_api_test.cpp

View workflow job for this annotation

GitHub Actions / lint

test/client_api_test.cpp:461:61 [readability-magic-numbers]

100 is a magic number; consider replacing it with a named constant
}
running = false;
});

swap_thread.join();
running = false;
evaluation_thread.join();
}
Loading