Skip to content

Commit 5909f33

Browse files
chrfwowchrfwow
committed
Flaky build due to a possible race condition #1449
Signed-off-by: christian.lutnik <[email protected]>
1 parent caae39d commit 5909f33

File tree

7 files changed

+107
-43
lines changed

7 files changed

+107
-43
lines changed

‎src/main/java/dev/openfeature/sdk/Awaitable.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package dev.openfeature.sdk;
2+
3+
public class Awaitable {
4+
public static final Awaitable FINISHED = new Awaitable(true);
5+
6+
private boolean isDone = false;
7+
8+
public Awaitable() {}
9+
10+
private Awaitable(boolean isDone) {
11+
this.isDone = isDone;
12+
}
13+
14+
public void await() {
15+
if (isDone) {
16+
return;
17+
}
18+
synchronized (this) {
19+
while (!isDone) {
20+
try {
21+
this.wait();
22+
} catch (InterruptedException ignored) {
23+
}
24+
}
25+
}
26+
}
27+
28+
public synchronized void wakeup() {
29+
isDone = true;
30+
this.notifyAll();
31+
}
32+
}

‎src/main/java/dev/openfeature/sdk/EventProvider.java‎

Lines changed: 41 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package dev.openfeature.sdk;
22

33
import dev.openfeature.sdk.internal.TriConsumer;
4+
import java.util.concurrent.ExecutorService;
5+
import java.util.concurrent.Executors;
6+
import java.util.concurrent.TimeUnit;
47
import lombok.extern.slf4j.Slf4j;
58

69
/**
@@ -18,6 +21,7 @@
1821
@Slf4j
1922
public abstract class EventProvider implements FeatureProvider {
2023
private EventProviderListener eventProviderListener;
24+
private final ExecutorService emitterExecutor = Executors.newCachedThreadPool();
2125

2226
void setEventProviderListener(EventProviderListener eventProviderListener) {
2327
this.eventProviderListener = eventProviderListener;
@@ -53,30 +57,49 @@ void detach() {
5357
* or timeout period has elapsed.
5458
*/
5559
@Override
56-
public void shutdown() {}
60+
public void shutdown() {
61+
emitterExecutor.shutdown();
62+
try {
63+
if (!emitterExecutor.awaitTermination(EventSupport.SHUTDOWN_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
64+
log.warn("Emitter executor did not terminate before the timeout period had elapsed");
65+
emitterExecutor.shutdownNow();
66+
}
67+
} catch (InterruptedException e) {
68+
emitterExecutor.shutdownNow();
69+
Thread.currentThread().interrupt();
70+
}
71+
}
5772

5873
/**
5974
* Emit the specified {@link ProviderEvent}.
6075
*
6176
* @param event The event type
6277
* @param details The details of the event
6378
*/
64-
public void emit(final ProviderEvent event, final ProviderEventDetails details) {
79+
public Awaitable emit(final ProviderEvent event, final ProviderEventDetails details) {
6580
final var localEventProviderListener = this.eventProviderListener;
6681
final var localOnEmit = this.onEmit;
6782

6883
if (localEventProviderListener == null && localOnEmit == null) {
69-
return;
84+
return Awaitable.FINISHED;
7085
}
7186

72-
try (var ignored = OpenFeatureAPI.lock.readLockAutoCloseable()) {
73-
if (localEventProviderListener != null) {
74-
localEventProviderListener.onEmit(event, details);
75-
}
76-
if (localOnEmit != null) {
77-
localOnEmit.accept(this, event, details);
87+
final var awaitable = new Awaitable();
88+
89+
emitterExecutor.submit(() -> {
90+
try (var ignored = OpenFeatureAPI.lock.readLockAutoCloseable()) {
91+
if (localEventProviderListener != null) {
92+
localEventProviderListener.onEmit(event, details);
93+
}
94+
if (localOnEmit != null) {
95+
localOnEmit.accept(this, event, details);
96+
}
97+
} finally {
98+
awaitable.wakeup();
7899
}
79-
}
100+
});
101+
102+
return awaitable;
80103
}
81104

82105
/**
@@ -85,8 +108,8 @@ public void emit(final ProviderEvent event, final ProviderEventDetails details)
85108
*
86109
* @param details The details of the event
87110
*/
88-
public void emitProviderReady(ProviderEventDetails details) {
89-
emit(ProviderEvent.PROVIDER_READY, details);
111+
public Awaitable emitProviderReady(ProviderEventDetails details) {
112+
return emit(ProviderEvent.PROVIDER_READY, details);
90113
}
91114

92115
/**
@@ -96,8 +119,8 @@ public void emitProviderReady(ProviderEventDetails details) {
96119
*
97120
* @param details The details of the event
98121
*/
99-
public void emitProviderConfigurationChanged(ProviderEventDetails details) {
100-
emit(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, details);
122+
public Awaitable emitProviderConfigurationChanged(ProviderEventDetails details) {
123+
return emit(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, details);
101124
}
102125

103126
/**
@@ -106,8 +129,8 @@ public void emitProviderConfigurationChanged(ProviderEventDetails details) {
106129
*
107130
* @param details The details of the event
108131
*/
109-
public void emitProviderStale(ProviderEventDetails details) {
110-
emit(ProviderEvent.PROVIDER_STALE, details);
132+
public Awaitable emitProviderStale(ProviderEventDetails details) {
133+
return emit(ProviderEvent.PROVIDER_STALE, details);
111134
}
112135

113136
/**
@@ -116,7 +139,7 @@ public void emitProviderStale(ProviderEventDetails details) {
116139
*
117140
* @param details The details of the event
118141
*/
119-
public void emitProviderError(ProviderEventDetails details) {
120-
emit(ProviderEvent.PROVIDER_ERROR, details);
142+
public Awaitable emitProviderError(ProviderEventDetails details) {
143+
return emit(ProviderEvent.PROVIDER_ERROR, details);
121144
}
122145
}

‎src/main/java/dev/openfeature/sdk/EventSupport.java‎

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
package dev.openfeature.sdk;
22

3-
import java.util.ArrayList;
4-
import java.util.List;
3+
import java.util.Collection;
54
import java.util.Map;
65
import java.util.Optional;
76
import java.util.Set;
87
import java.util.UUID;
98
import java.util.concurrent.ConcurrentHashMap;
9+
import java.util.concurrent.ConcurrentLinkedQueue;
1010
import java.util.concurrent.ExecutorService;
1111
import java.util.concurrent.Executors;
1212
import java.util.concurrent.TimeUnit;
@@ -23,13 +23,10 @@ class EventSupport {
2323

2424
// we use a v4 uuid as a "placeholder" for anonymous clients, since
2525
// ConcurrentHashMap doesn't support nulls
26-
private static final String defaultClientUuid = UUID.randomUUID().toString();
26+
private static final String DEFAULT_CLIENT_UUID = UUID.randomUUID().toString();
2727
private final Map<String, HandlerStore> handlerStores = new ConcurrentHashMap<>();
2828
private final HandlerStore globalHandlerStore = new HandlerStore();
29-
private final ExecutorService taskExecutor = Executors.newCachedThreadPool(runnable -> {
30-
final Thread thread = new Thread(runnable);
31-
return thread;
32-
});
29+
private final ExecutorService taskExecutor = Executors.newCachedThreadPool();
3330

3431
/**
3532
* Run all the event handlers associated with this domain.
@@ -40,11 +37,10 @@ class EventSupport {
4037
* @param eventDetails the event details
4138
*/
4239
public void runClientHandlers(String domain, ProviderEvent event, EventDetails eventDetails) {
43-
domain = Optional.ofNullable(domain).orElse(defaultClientUuid);
40+
domain = Optional.ofNullable(domain).orElse(DEFAULT_CLIENT_UUID);
4441

4542
// run handlers if they exist
4643
Optional.ofNullable(handlerStores.get(domain))
47-
.filter(store -> Optional.of(store).isPresent())
4844
.map(store -> store.handlerMap.get(event))
4945
.ifPresent(handlers -> handlers.forEach(handler -> runHandler(handler, eventDetails)));
5046
}
@@ -69,7 +65,7 @@ public void runGlobalHandlers(ProviderEvent event, EventDetails eventDetails) {
6965
* @param handler the handler function to run
7066
*/
7167
public void addClientHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
72-
final String name = Optional.ofNullable(domain).orElse(defaultClientUuid);
68+
final String name = Optional.ofNullable(domain).orElse(DEFAULT_CLIENT_UUID);
7369

7470
// lazily create and cache a HandlerStore if it doesn't exist
7571
HandlerStore store = Optional.ofNullable(this.handlerStores.get(name)).orElseGet(() -> {
@@ -89,7 +85,7 @@ public void addClientHandler(String domain, ProviderEvent event, Consumer<EventD
8985
* @param handler the handler ref to be removed
9086
*/
9187
public void removeClientHandler(String domain, ProviderEvent event, Consumer<EventDetails> handler) {
92-
domain = Optional.ofNullable(domain).orElse(defaultClientUuid);
88+
domain = Optional.ofNullable(domain).orElse(DEFAULT_CLIENT_UUID);
9389
this.handlerStores.get(domain).removeHandler(event, handler);
9490
}
9591

@@ -160,14 +156,14 @@ public void shutdown() {
160156
// instantiated when a handler is added to that client.
161157
static class HandlerStore {
162158

163-
private final Map<ProviderEvent, List<Consumer<EventDetails>>> handlerMap;
159+
private final Map<ProviderEvent, Collection<Consumer<EventDetails>>> handlerMap;
164160

165161
HandlerStore() {
166162
handlerMap = new ConcurrentHashMap<>();
167-
handlerMap.put(ProviderEvent.PROVIDER_READY, new ArrayList<>());
168-
handlerMap.put(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, new ArrayList<>());
169-
handlerMap.put(ProviderEvent.PROVIDER_ERROR, new ArrayList<>());
170-
handlerMap.put(ProviderEvent.PROVIDER_STALE, new ArrayList<>());
163+
handlerMap.put(ProviderEvent.PROVIDER_READY, new ConcurrentLinkedQueue<>());
164+
handlerMap.put(ProviderEvent.PROVIDER_CONFIGURATION_CHANGED, new ConcurrentLinkedQueue<>());
165+
handlerMap.put(ProviderEvent.PROVIDER_ERROR, new ConcurrentLinkedQueue<>());
166+
handlerMap.put(ProviderEvent.PROVIDER_STALE, new ConcurrentLinkedQueue<>());
171167
}
172168

173169
void addHandler(ProviderEvent event, Consumer<EventDetails> handler) {

‎src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ void shouldPutTheProviderInStateErrorAfterEmittingErrorEvent() {
150150
api.setProviderAndWait(domain, provider);
151151
Client client = api.getClient(domain);
152152
assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
153-
provider.emitProviderError(ProviderEventDetails.builder().build());
153+
provider.emitProviderError(ProviderEventDetails.builder().build()).await();
154154
assertThat(client.getProviderState()).isEqualTo(ProviderState.ERROR);
155155
}
156156

@@ -165,7 +165,7 @@ void shouldPutTheProviderInStateStaleAfterEmittingStaleEvent() {
165165
api.setProviderAndWait(domain, provider);
166166
Client client = api.getClient(domain);
167167
assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
168-
provider.emitProviderStale(ProviderEventDetails.builder().build());
168+
provider.emitProviderStale(ProviderEventDetails.builder().build()).await();
169169
assertThat(client.getProviderState()).isEqualTo(ProviderState.STALE);
170170
}
171171

@@ -180,9 +180,9 @@ void shouldPutTheProviderInStateReadyAfterEmittingReadyEvent() {
180180
api.setProviderAndWait(domain, provider);
181181
Client client = api.getClient(domain);
182182
assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
183-
provider.emitProviderStale(ProviderEventDetails.builder().build());
183+
provider.emitProviderStale(ProviderEventDetails.builder().build()).await();
184184
assertThat(client.getProviderState()).isEqualTo(ProviderState.STALE);
185-
provider.emitProviderReady(ProviderEventDetails.builder().build());
185+
provider.emitProviderReady(ProviderEventDetails.builder().build()).await();
186186
assertThat(client.getProviderState()).isEqualTo(ProviderState.READY);
187187
}
188188
}

‎src/test/java/dev/openfeature/sdk/EventProviderTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ public static void resetDefaultProvider() {
3232
}
3333

3434
@Test
35+
@Timeout(2)
3536
@DisplayName("should run attached onEmit with emitters")
3637
void emitsEventsWhenAttached() {
3738
TriConsumer<EventProvider, ProviderEvent, ProviderEventDetails> onEmit = mockOnEmit();

‎src/test/java/dev/openfeature/sdk/EventsTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -604,7 +604,7 @@ void matchingStaleEventsMustRunImmediately() {
604604
TestEventsProvider provider = new TestEventsProvider(INIT_DELAY);
605605
Client client = api.getClient(name);
606606
api.setProviderAndWait(name, provider);
607-
provider.emitProviderStale(ProviderEventDetails.builder().build());
607+
provider.emitProviderStale(ProviderEventDetails.builder().build()).await();
608608
assertThat(client.getProviderState()).isEqualTo(ProviderState.STALE);
609609

610610
// should run even though handler was added after stale
@@ -625,7 +625,7 @@ void matchingErrorEventsMustRunImmediately() {
625625
TestEventsProvider provider = new TestEventsProvider(INIT_DELAY);
626626
Client client = api.getClient(name);
627627
api.setProviderAndWait(name, provider);
628-
provider.emitProviderError(ProviderEventDetails.builder().build());
628+
provider.emitProviderError(ProviderEventDetails.builder().build()).await();
629629
assertThat(client.getProviderState()).isEqualTo(ProviderState.ERROR);
630630

631631
verify(handler, never()).accept(any());

‎src/test/java/dev/openfeature/sdk/providers/memory/InMemoryProviderTest.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,14 @@
33
import static dev.openfeature.sdk.Structure.mapToStructure;
44
import static dev.openfeature.sdk.testutils.TestFlagsUtils.buildFlags;
55
import static org.awaitility.Awaitility.await;
6-
import static org.junit.jupiter.api.Assertions.*;
6+
import static org.junit.jupiter.api.Assertions.assertEquals;
7+
import static org.junit.jupiter.api.Assertions.assertThrows;
8+
import static org.junit.jupiter.api.Assertions.assertTrue;
79
import static org.mockito.ArgumentMatchers.argThat;
8-
import static org.mockito.Mockito.*;
10+
import static org.mockito.Mockito.mock;
11+
import static org.mockito.Mockito.spy;
12+
import static org.mockito.Mockito.times;
13+
import static org.mockito.Mockito.verify;
914

1015
import com.google.common.collect.ImmutableMap;
1116
import dev.openfeature.sdk.Client;
@@ -19,6 +24,7 @@
1924
import dev.openfeature.sdk.exceptions.TypeMismatchError;
2025
import java.util.HashMap;
2126
import java.util.Map;
27+
import java.util.concurrent.atomic.AtomicInteger;
2228
import java.util.function.Consumer;
2329
import lombok.SneakyThrows;
2430
import org.junit.jupiter.api.BeforeEach;
@@ -34,10 +40,11 @@ class InMemoryProviderTest {
3440
@SneakyThrows
3541
@BeforeEach
3642
void beforeEach() {
43+
final var configChangedEventCounter = new AtomicInteger();
3744
Map<String, Flag<?>> flags = buildFlags();
3845
provider = spy(new InMemoryProvider(flags));
3946
api = OpenFeatureAPITestUtil.createAPI();
40-
api.onProviderConfigurationChanged(eventDetails -> {});
47+
api.onProviderConfigurationChanged(eventDetails -> configChangedEventCounter.incrementAndGet());
4148
api.setProviderAndWait(provider);
4249
client = api.getClient();
4350
provider.updateFlags(flags);
@@ -48,6 +55,11 @@ void beforeEach() {
4855
.variant("off", false)
4956
.defaultVariant("on")
5057
.build());
58+
59+
// wait for the two config changed events to be fired, otherwise they could mess with our tests
60+
while (configChangedEventCounter.get() < 2) {
61+
Thread.sleep(1);
62+
}
5163
}
5264

5365
@Test

0 commit comments

Comments
 (0)