feat: allow sdk consumers to choose their synchronization strategy #16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
greghuels
force-pushed
the
greg.huels/FFESUPPORT-331/mutex
branch
from
dbe539b to
93076cb
Compare
greghuels
changed the title
aarsilv
approved these changes
Nov 25, 2025
Contributor
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
Looks good small actual change then tons of changes from dereferencing to direct access 😛
| auto applicationLogger = std::make_shared<MyApplicationLogger>(); | ||
|
|
||
| auto client = std::make_shared<eppoclient::EppoClient>( | ||
| eppoclient::EppoClient client( |
Contributor
There was a problem hiding this comment.
Ah ok this is why we change from dereferencing to access in the README 👍
Comment on lines
+640
to
+642
| ## Thread Safety and Concurrency | ||
|
|
||
| The Eppo C++ SDK is **not thread-safe by design**. If you need to use the SDK from multiple threads, you are responsible for providing appropriate synchronization mechanisms in your application. |
| bool feature2 = client.getBoolAssignment("flag2", "user-123", attrs, false); | ||
| ``` | ||
| ### Multi-Threaded Usage (Synchronization Required) |
Contributor
There was a problem hiding this comment.
Nice thinking including this
|
|
||
| Configuration ConfigurationStore::getConfiguration() const { | ||
| // Lock and get a copy of the shared_ptr | ||
| std::lock_guard<std::mutex> lock(configMutex_); |
test/test_configuration_store.cpp
Outdated
Comment on lines
28
to
29
| // Thread safety test removed - ConfigurationStore is no longer thread-safe by design. | ||
| // If thread-safety is required, the caller is responsible for providing synchronization. |
Contributor
There was a problem hiding this comment.
This is covered in the README unsure if you need this in the test file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Eppo Internal:
🎟️ Fixes FFESUPPORT-331
Motivation and Context
We do not want to assume mutex-based synchronization so that we can let customers handle concurrency in whatever way their project requires.
Feedback from customer:
Description
Update code so that it runs as a single-threaded, letting customers handle concurrency.
How has this been documented?
README.md has been updated to reflect these changes
How has this been tested?
Tests updated and run, including example applications