Skip to content

feat: add means of awaiting event emission, fix flaky build #1463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jun 10, 2025
Merged

feat: add means of awaiting event emission, fix flaky build #1463

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
306b74f
Flaky build due to a possible race condition #1449
chrfwow May 28, 2025
5bd4586
Flaky build due to a possible race condition #1449
chrfwow May 28, 2025
caae39d
Flaky build due to a possible race condition #1449
chrfwow May 28, 2025
5909f33
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
43c06f3
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
9e2cbd8
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
694526e
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
95d2271
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
0738f9f
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
3a1ce09
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
5ab1f68
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
1feacdd
Flaky build due to a possible race condition #1449
chrfwow Jun 2, 2025
6a4fa3d
Flaky build due to a possible race condition #1449
chrfwow Jun 3, 2025
c22da3e
Merge branch 'main' into Flaky-build-due-to-a-possible-race-condition…
toddbaert Jun 9, 2025
14d8c12
Merge branch 'main' into Flaky-build-due-to-a-possible-race-condition…
toddbaert Jun 10, 2025
596fe22
Update src/main/java/dev/openfeature/sdk/Awaitable.java
toddbaert Jun 10, 2025
0228a70
Update src/main/java/dev/openfeature/sdk/Awaitable.java
toddbaert Jun 10, 2025
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
30 changes: 13 additions & 17 deletions src/main/java/dev/openfeature/sdk/EventProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
@Slf4j
public abstract class EventProvider implements FeatureProvider {
private EventProviderListener eventProviderListener;
private final ExecutorService emitterExecutor = Executors.newCachedThreadPool();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrong, we do need it


void setEventProviderListener(EventProviderListener eventProviderListener) {
this.eventProviderListener = eventProviderListener;
Expand Down Expand Up @@ -58,16 +57,6 @@ void detach() {
*/
@Override
public void shutdown() {
emitterExecutor.shutdown();
try {
if (!emitterExecutor.awaitTermination(EventSupport.SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
log.warn("Emitter executor did not terminate before the timeout period had elapsed");
emitterExecutor.shutdownNow();
}
} catch (InterruptedException e) {
emitterExecutor.shutdownNow();
Thread.currentThread().interrupt();
}
}

/**
Expand All @@ -76,14 +65,21 @@ public void shutdown() {
* @param event The event type
* @param details The details of the event
*/
public void emit(ProviderEvent event, ProviderEventDetails details) {
if (eventProviderListener != null) {
eventProviderListener.onEmit(event, details);
public void emit(final ProviderEvent event, final ProviderEventDetails details) {
final var localEventProviderListener = this.eventProviderListener;
final var localOnEmit = this.onEmit;

if (localEventProviderListener == null && localOnEmit == null) {
return;
}

final TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> localOnEmit = this.onEmit;
if (localOnEmit != null) {
emitterExecutor.submit(() -> localOnEmit.accept(this, event, details));
try (var ignored = OpenFeatureAPI.lock.readLockAutoCloseable()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not nice to access the static lock that's clearly only exposed for testing, but we need to use this very lock to protect against concurrent reads/writes to the underlying map.

I removed the call to the emitterExecutor, so now all listeners are invoked sequentially on the calling thread. This was needed as several tests expect the state of the provider to be set immediately after calling this method.
If we consider this a problem, I could work around this issue by returning something our tests can wait for. In this case, we would need to call both the localOnEmit and the localEventProviderListener in the emitterExecutor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By moving the call from the emitterExecutor to the current thread, I reintroduced a race condition that was already fixed.
Therefore I added the executor again. This means that some tests will fail that expect the state of the provider to be updated immediately after a call to emit. This is why I now return an Awaitable, for which those tests (or users) can wait.

if (localEventProviderListener != null) {
localEventProviderListener.onEmit(event, details);
}
if (localOnEmit != null) {
localOnEmit.accept(this, event, details);
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/test/java/dev/openfeature/sdk/EventsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class EventsTest {
private OpenFeatureAPI api;

@BeforeEach
public void setUp() throws Exception {
void setUp() {
api = new OpenFeatureAPI();
}

Expand Down Expand Up @@ -578,7 +578,7 @@ void shouldHaveAllProperties() {
number = "5.3.3",
text = "Handlers attached after the provider is already in the associated state, MUST run immediately.")
void matchingReadyEventsMustRunImmediately() {
final String name = "matchingEventsMustRunImmediately";
final String name = "matchingReadyEventsMustRunImmediately";
final Consumer<EventDetails> handler = mockHandler();

// provider which is already ready
Expand All @@ -597,7 +597,7 @@ void matchingReadyEventsMustRunImmediately() {
number = "5.3.3",
text = "Handlers attached after the provider is already in the associated state, MUST run immediately.")
void matchingStaleEventsMustRunImmediately() {
final String name = "matchingEventsMustRunImmediately";
final String name = "matchingStaleEventsMustRunImmediately";
final Consumer<EventDetails> handler = mockHandler();

// provider which is already stale
Expand All @@ -618,7 +618,7 @@ void matchingStaleEventsMustRunImmediately() {
number = "5.3.3",
text = "Handlers attached after the provider is already in the associated state, MUST run immediately.")
void matchingErrorEventsMustRunImmediately() {
final String name = "matchingEventsMustRunImmediately";
final String name = "matchingErrorEventsMustRunImmediately";
final Consumer<EventDetails> handler = mockHandler();

// provider which is already in error
Expand Down
Loading