From 90b109e864d79343ec05a313924b0ee6060b2a64 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 30 Sep 2024 14:46:30 -0700 Subject: [PATCH 1/4] feat: add client-side C binding for fetching data source state --- .../launchdarkly/client_side/bindings/c/sdk.h | 16 +++++++++++- .../client_side/data_source_status.hpp | 19 ++++++++------ libs/client-sdk/src/bindings/c/sdk.cpp | 8 ++++++ .../src/data_sources/data_source_status.cpp | 25 ++++++++++--------- .../tests/client_c_bindings_test.cpp | 21 ++++++++++++++++ 5 files changed, 68 insertions(+), 21 deletions(-) diff --git a/libs/client-sdk/include/launchdarkly/client_side/bindings/c/sdk.h b/libs/client-sdk/include/launchdarkly/client_side/bindings/c/sdk.h index e45ba26ad..7ed7a4830 100644 --- a/libs/client-sdk/include/launchdarkly/client_side/bindings/c/sdk.h +++ b/libs/client-sdk/include/launchdarkly/client_side/bindings/c/sdk.h @@ -501,9 +501,23 @@ enum LDDataSourceStatus_State { * SDK key will never become valid), or because the SDK client was * explicitly shut down. */ - LD_DATASOURCESTATUS_STATE_SHUTDOWN = 4 + LD_DATASOURCESTATUS_STATE_SHUTDOWN = 4, + + LD_DATASOURCESTATUS_STATE_UNUSED_MAXVALUE = + INT32_MAX /* Used to ensure the underlying type is + * at least 32 bits. */ }; +/** + * @param state The state to convert to a string. + * @param default_if_unknown The default string to return if the state is not + * recognized. + * @return Returns the name of the given LDDataSourceStatus_State. + */ +LD_EXPORT(char const*) +LDDataSourceStatus_State_Name(enum LDDataSourceStatus_State state, + char const* default_if_unknown); + /** * Get an enumerated value representing the overall current state of the data * source. diff --git a/libs/client-sdk/include/launchdarkly/client_side/data_source_status.hpp b/libs/client-sdk/include/launchdarkly/client_side/data_source_status.hpp index 4c14572c5..6909ecdd1 100644 --- a/libs/client-sdk/include/launchdarkly/client_side/data_source_status.hpp +++ b/libs/client-sdk/include/launchdarkly/client_side/data_source_status.hpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -15,7 +16,7 @@ namespace launchdarkly::client_side::data_sources { /** * Enumeration of possible data source states. */ -enum class DataSourceState { +enum class DataSourceState : std::int32_t { /** * The initial state of the data source when the SDK is being * initialized. @@ -62,15 +63,17 @@ enum class DataSourceState { * SDK key will never become valid), or because the SDK client was * explicitly shut down. */ - kShutdown = 4, - - // BackgroundDisabled, - - // TODO: A plugin of sorts would likely be required to implement - // network availability. - // kNetworkUnavailable, + kShutdown = 4 }; +/** + * + * @return Returns the name of the given DataSourceState as a string. If + * the enum value is not recognized, the default string value is returned. + */ +char const* GetDataSourceStateName(DataSourceState state, + char const* default_if_unknown); + using DataSourceStatus = common::data_sources::DataSourceStatusBase; diff --git a/libs/client-sdk/src/bindings/c/sdk.cpp b/libs/client-sdk/src/bindings/c/sdk.cpp index da070b98e..078a4ba55 100644 --- a/libs/client-sdk/src/bindings/c/sdk.cpp +++ b/libs/client-sdk/src/bindings/c/sdk.cpp @@ -352,6 +352,14 @@ LDDataSourceStatus_GetState(LDDataSourceStatus status) { TO_DATASOURCESTATUS(status)->State()); } +LD_EXPORT(char const*) +LDDataSourceStatus_State_Name(enum LDDataSourceStatus_State state, + char const* default_if_unknown) { + return GetDataSourceStateName( + static_cast(state), + default_if_unknown); +} + LD_EXPORT(LDDataSourceStatus_ErrorInfo) LDDataSourceStatus_GetLastError(LDDataSourceStatus status) { LD_ASSERT_NOT_NULL(status); diff --git a/libs/client-sdk/src/data_sources/data_source_status.cpp b/libs/client-sdk/src/data_sources/data_source_status.cpp index 437b5b357..98446622d 100644 --- a/libs/client-sdk/src/data_sources/data_source_status.cpp +++ b/libs/client-sdk/src/data_sources/data_source_status.cpp @@ -4,26 +4,27 @@ namespace launchdarkly::client_side::data_sources { -std::ostream& operator<<(std::ostream& out, - DataSourceStatus::DataSourceState const& state) { +char const* GetDataSourceStateName(DataSourceState state, + char const* default_if_unknown) { switch (state) { case DataSourceStatus::DataSourceState::kInitializing: - out << "INITIALIZING"; - break; + return "INITIALIZING"; case DataSourceStatus::DataSourceState::kValid: - out << "VALID"; - break; + return "VALID"; case DataSourceStatus::DataSourceState::kInterrupted: - out << "INTERRUPTED"; - break; + return "INTERRUPTED"; case DataSourceStatus::DataSourceState::kSetOffline: - out << "OFFLINE"; - break; + return "OFFLINE"; case DataSourceStatus::DataSourceState::kShutdown: - out << "SHUTDOWN"; - break; + return "SHUTDOWN"; + default: + return default_if_unknown; } +} +std::ostream& operator<<(std::ostream& out, + DataSourceStatus::DataSourceState const& state) { + out << GetDataSourceStateName(state, "UNKNOWN"); return out; } diff --git a/libs/client-sdk/tests/client_c_bindings_test.cpp b/libs/client-sdk/tests/client_c_bindings_test.cpp index 42f5dac0c..f0b91cc35 100644 --- a/libs/client-sdk/tests/client_c_bindings_test.cpp +++ b/libs/client-sdk/tests/client_c_bindings_test.cpp @@ -194,3 +194,24 @@ TEST(ClientBindings, ComplexDataSourceStatus) { LDDataSourceStatus_ErrorInfo_Free(info); } + +TEST(ClientBindings, TestDataSourceStatusStateName) { + ASSERT_STREQ(LDDataSourceStatus_State_Name(LD_DATASOURCESTATUS_STATE_VALID, + "unknown"), + "VALID"); + ASSERT_STREQ(LDDataSourceStatus_State_Name( + LD_DATASOURCESTATUS_STATE_OFFLINE, "unknown"), + "OFFLINE"); + ASSERT_STREQ(LDDataSourceStatus_State_Name( + LD_DATASOURCESTATUS_STATE_INITIALIZING, "unknown"), + "INITIALIZING"); + ASSERT_STREQ(LDDataSourceStatus_State_Name( + LD_DATASOURCESTATUS_STATE_SHUTDOWN, "unknown"), + "SHUTDOWN"); + ASSERT_STREQ(LDDataSourceStatus_State_Name( + LD_DATASOURCESTATUS_STATE_INTERRUPTED, "unknown"), + "INTERRUPTED"); + ASSERT_STREQ(LDDataSourceStatus_State_Name( + LD_DATASOURCESTATUS_STATE_UNUSED_MAXVALUE, "unknown"), + "unknown"); +} From 54a2a9cbf82fe1c7841328c3b8e267b41d1e7821 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 30 Sep 2024 12:48:19 -0700 Subject: [PATCH 2/4] fix: improve handling of streaming error state changes/logging (#439) The existing handling for data source state transitions when receiving `sse` errors was incorrect. It was treating any error as a state transition to `kOff/kShutdown`, and logging at the `error` level. The correct behavior would be making that transition only if the error is unrecoverable. Additionally, recoverable errors should be logged at the debug level, in order to not overstate the impact of the issue. This bug did not actually affect the backoff behavior of the eventsource, but it did impact status listeners and any code that queried the existing status within the SDK. --- libs/client-sdk/src/client_impl.cpp | 18 ++++++++---------- libs/client-sdk/src/client_impl.hpp | 5 +---- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index 9e2f66075..a0cdb3173 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -145,8 +145,7 @@ static bool IsInitializedSuccessfully(DataSourceStatus::DataSourceState state) { // Was any attempt made to initialize the data source (with a successful or // permanent failure outcome?) static bool IsInitialized(DataSourceStatus::DataSourceState state) { - return IsInitializedSuccessfully(state) || - (state == DataSourceStatus::DataSourceState::kShutdown); + return state != DataSourceStatus::DataSourceState::kInitializing; } std::future ClientImpl::IdentifyAsync(Context context) { @@ -155,7 +154,7 @@ std::future ClientImpl::IdentifyAsync(Context context) { event_processor_->SendAsync(events::IdentifyEventParams{ std::chrono::system_clock::now(), std::move(context)}); - return StartAsyncInternal(IsInitializedSuccessfully); + return StartAsyncInternal(); } void ClientImpl::RestartDataSource() { @@ -169,16 +168,15 @@ void ClientImpl::RestartDataSource() { data_source_->ShutdownAsync(start_op); } -std::future ClientImpl::StartAsyncInternal( - std::function result_predicate) { +std::future ClientImpl::StartAsyncInternal() { auto init_promise = std::make_shared>(); auto init_future = init_promise->get_future(); status_manager_.OnDataSourceStatusChangeEx( - [result = std::move(result_predicate), - init_promise](data_sources::DataSourceStatus const& status) { + [init_promise](data_sources::DataSourceStatus const& status) { if (auto const state = status.State(); IsInitialized(state)) { - init_promise->set_value(result(status.State())); + init_promise->set_value( + IsInitializedSuccessfully(status.State())); return true; /* delete this change listener since the desired state was reached */ } @@ -191,11 +189,11 @@ std::future ClientImpl::StartAsyncInternal( } std::future ClientImpl::StartAsync() { - return StartAsyncInternal(IsInitializedSuccessfully); + return StartAsyncInternal(); } bool ClientImpl::Initialized() const { - return IsInitializedSuccessfully(status_manager_.Status().State()); + return IsInitialized(status_manager_.Status().State()); } std::unordered_map ClientImpl::AllFlags() const { diff --git a/libs/client-sdk/src/client_impl.hpp b/libs/client-sdk/src/client_impl.hpp index 94155501b..78142e347 100644 --- a/libs/client-sdk/src/client_impl.hpp +++ b/libs/client-sdk/src/client_impl.hpp @@ -109,10 +109,7 @@ class ClientImpl : public IClient { void RestartDataSource(); - std::future StartAsyncInternal( - std::function - predicate); - + std::future StartAsyncInternal(); Config config_; Logger logger_; From 3a57bb98fecb0425a621e58734b8374f20c95506 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 30 Sep 2024 15:37:17 -0700 Subject: [PATCH 3/4] change IsInitializedSuccessfully condition to handle interrupted state --- libs/client-sdk/src/client_impl.cpp | 35 +++++++++++++++++++---------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/libs/client-sdk/src/client_impl.cpp b/libs/client-sdk/src/client_impl.cpp index a0cdb3173..d99bd72a5 100644 --- a/libs/client-sdk/src/client_impl.cpp +++ b/libs/client-sdk/src/client_impl.cpp @@ -134,18 +134,28 @@ ClientImpl::ClientImpl(Config in_cfg, run_thread_ = std::move(std::thread([&]() { ioc_.run(); })); } -// Was an attempt made to initialize the data source, and did that attempt -// succeed? The data source being connected, or not being connected due to -// offline mode, both represent successful terminal states. +// Returns true if the SDK can be considered initialized. We have defined +// explicit configuration of offline mode as initialized. +// +// When online, we're initialized if we've obtained a payload and are healthy +// (kValid) or obtained a payload and are unhealthy (kInterrupted). +// +// The purpose of this concept is to enable: +// +// (1) Resolving the StartAsync() promise. Once the SDK is no longer +// initializing, this promise needs to indicate if the process was successful +// or not. +// +// (2) Providing a getter for (1), if the user didn't check the promise or +// otherwise need to poll the state. That's the Initialized() method. +// +// (3) As a diagnostic during evaluation, to log a message warning that a +// cached (if persistence is being used) or default (if not) value will be +// returned because the SDK is not yet initialized. static bool IsInitializedSuccessfully(DataSourceStatus::DataSourceState state) { return (state == DataSourceStatus::DataSourceState::kValid || - state == DataSourceStatus::DataSourceState::kSetOffline); -} - -// Was any attempt made to initialize the data source (with a successful or -// permanent failure outcome?) -static bool IsInitialized(DataSourceStatus::DataSourceState state) { - return state != DataSourceStatus::DataSourceState::kInitializing; + state == DataSourceStatus::DataSourceState::kSetOffline || + state == DataSourceStatus::DataSourceState::kInterrupted); } std::future ClientImpl::IdentifyAsync(Context context) { @@ -174,7 +184,8 @@ std::future ClientImpl::StartAsyncInternal() { status_manager_.OnDataSourceStatusChangeEx( [init_promise](data_sources::DataSourceStatus const& status) { - if (auto const state = status.State(); IsInitialized(state)) { + if (auto const state = status.State(); + state != DataSourceStatus::DataSourceState::kInitializing) { init_promise->set_value( IsInitializedSuccessfully(status.State())); return true; /* delete this change listener since the desired @@ -193,7 +204,7 @@ std::future ClientImpl::StartAsync() { } bool ClientImpl::Initialized() const { - return IsInitialized(status_manager_.Status().State()); + return IsInitializedSuccessfully(status_manager_.Status().State()); } std::unordered_map ClientImpl::AllFlags() const { From 1626450aee6aff24949ae07d3d0daad38a0f4991 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Mon, 30 Sep 2024 15:53:33 -0700 Subject: [PATCH 4/4] update unit tests --- libs/client-sdk/tests/client_c_bindings_test.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/client-sdk/tests/client_c_bindings_test.cpp b/libs/client-sdk/tests/client_c_bindings_test.cpp index f0b91cc35..82bb0e0f5 100644 --- a/libs/client-sdk/tests/client_c_bindings_test.cpp +++ b/libs/client-sdk/tests/client_c_bindings_test.cpp @@ -148,6 +148,8 @@ TEST(ClientBindings, GetStatusOfOfflineClient) { bool success = false; LDClientSDK_Start(sdk, 3000, &success); + EXPECT_TRUE(success); + LDDataSourceStatus status_2 = LDClientSDK_DataSourceStatus_Status(sdk); EXPECT_EQ(LD_DATASOURCESTATUS_STATE_OFFLINE, LDDataSourceStatus_GetState(status_2)); @@ -156,6 +158,8 @@ TEST(ClientBindings, GetStatusOfOfflineClient) { EXPECT_NE(0, LDDataSourceStatus_StateSince(status_2)); + EXPECT_TRUE(LDClientSDK_Initialized(sdk)); + LDDataSourceStatus_Free(status_1); LDDataSourceStatus_Free(status_2); LDClientSDK_Free(sdk);