Skip to content

Commit 87af314

Browse files
dd-oleksiidd-oleksii
committed
fix: merge SetProviderAndWait fixes into next
Signed-off-by: Oleksii Shmalko <oleksii.shmalko@datadoghq.com>
2 parents 05198d3 + 4576e34 commit 87af314

File tree

6 files changed

+116
-153
lines changed

6 files changed

+116
-153
lines changed

‎event_executor.go‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,11 @@
11
package openfeature
22

33
import (
4-
"fmt"
54
"log/slog"
65
"maps"
76
reflect "reflect"
87
"slices"
98
"sync"
10-
"time"
119
)
1210

1311
const defaultDomain = ""
@@ -237,32 +235,32 @@ func (e *eventExecutor) State(domain string) State {
237235
}
238236

239237
// registerDefaultProvider registers the default FeatureProvider and remove the old default provider if available
240-
func (e *eventExecutor) registerDefaultProvider(provider FeatureProvider) error {
238+
func (e *eventExecutor) registerDefaultProvider(provider FeatureProvider) {
241239
e.mu.Lock()
242240
defer e.mu.Unlock()
243241

244242
newProvider := newProviderRef(provider)
245243
oldProvider := e.defaultProviderReference
246244
e.defaultProviderReference = newProvider
247245

248-
return e.startListeningAndShutdownOld(newProvider, oldProvider)
246+
e.startListeningAndShutdownOld(newProvider, oldProvider)
249247
}
250248

251249
// registerNamedEventingProvider registers a named FeatureProvider and remove event listener for old named provider
252-
func (e *eventExecutor) registerNamedEventingProvider(associatedClient string, provider FeatureProvider) error {
250+
func (e *eventExecutor) registerNamedEventingProvider(associatedClient string, provider FeatureProvider) {
253251
e.mu.Lock()
254252
defer e.mu.Unlock()
255253
newProvider := newProviderRef(provider)
256254

257255
oldProvider := e.namedProviderReference[associatedClient]
258256
e.namedProviderReference[associatedClient] = newProvider
259257

260-
return e.startListeningAndShutdownOld(newProvider, oldProvider)
258+
e.startListeningAndShutdownOld(newProvider, oldProvider)
261259
}
262260

263261
// startListeningAndShutdownOld is a helper to start concurrent listening to new provider events and invoke shutdown
264262
// hook of the old provider if it's not bound by another subscription
265-
func (e *eventExecutor) startListeningAndShutdownOld(newProvider providerReference, oldReference providerReference) error {
263+
func (e *eventExecutor) startListeningAndShutdownOld(newProvider providerReference, oldReference providerReference) {
266264
// check if this provider already actively handled - 1:N binding capability
267265
if !isRunning(newProvider, e.activeSubscriptions) {
268266
e.activeSubscriptions = append(e.activeSubscriptions, newProvider)
@@ -292,7 +290,7 @@ func (e *eventExecutor) startListeningAndShutdownOld(newProvider providerReferen
292290

293291
// check if this provider is still bound - 1:N binding capability
294292
if isBound(oldReference, e.defaultProviderReference, slices.Collect(maps.Values(e.namedProviderReference))) {
295-
return nil
293+
return
296294
}
297295

298296
// drop from active references
@@ -303,16 +301,29 @@ func (e *eventExecutor) startListeningAndShutdownOld(newProvider providerReferen
303301
_, ok := oldReference.featureProvider.(EventHandler)
304302
if !ok {
305303
// no shutdown for non event handling provider
306-
return nil
304+
return
307305
}
308306

309307
// avoid shutdown lockouts
310308
select {
311309
case oldReference.shutdownSemaphore <- "":
312-
return nil
313-
case <-time.After(200 * time.Millisecond):
314-
return fmt.Errorf("old event handler %s timeout waiting for handler shutdown",
315-
oldReference.featureProvider.Metadata().Name)
310+
return
311+
default:
312+
// This should never happen:
313+
//
314+
// providerReference.shutdownSemaphore is created with
315+
// a buffer size of 1, so it should allow sending at
316+
// least one shutdown signal without blocking. Locking
317+
// should prevent us from sending more than one
318+
// signal.
319+
//
320+
// In the unlikely case that it does not, this
321+
// shouldn't be a big deal: we have already swapped
322+
// references in eventExecutor and openfeatureAPI, and
323+
// the handler should be able to receive at least one
324+
// shutdown signal later.
325+
slog.Info("OF BUG: failed to send shutdown to old event handler",
326+
"provider", oldReference.featureProvider.Metadata().Name)
316327
}
317328
}
318329

‎event_executor_test.go‎

Lines changed: 20 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,13 @@ func TestEventHandler_RegisterUnregisterEventProvider(t *testing.T) {
2828
}
2929

3030
executor := newEventExecutor()
31-
err := executor.registerDefaultProvider(eventingProvider)
32-
if err != nil {
33-
t.Fatal(err)
34-
}
31+
executor.registerDefaultProvider(eventingProvider)
3532

3633
if executor.defaultProviderReference.featureProvider != eventingProvider {
3734
t.Error("implementation should register default eventing provider")
3835
}
3936

40-
err = executor.registerNamedEventingProvider("domain", eventingProvider)
41-
if err != nil {
42-
t.Fatal(err)
43-
}
37+
executor.registerNamedEventingProvider("domain", eventingProvider)
4438

4539
if _, ok := executor.namedProviderReference["domain"]; !ok {
4640
t.Errorf("implementation should register named eventing provider")
@@ -959,7 +953,6 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) {
959953
eventingImpl := &ProviderEventing{
960954
c: make(chan Event, 1),
961955
}
962-
eventingImpl.Invoke(Event{EventType: ProviderError})
963956

964957
provider := struct {
965958
FeatureProvider
@@ -983,6 +976,9 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) {
983976
AddHandler(ProviderStale, callback)
984977
AddHandler(ProviderConfigChange, callback)
985978

979+
// Emit error event from the provider
980+
eventingImpl.Invoke(Event{EventType: ProviderError})
981+
986982
// assert client transitioned to ERROR
987983
eventually(t, func() bool {
988984
return NewClient().State() == ErrorState
@@ -1002,7 +998,6 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) {
1002998
eventingImpl := &ProviderEventing{
1003999
c: make(chan Event, 1),
10041000
}
1005-
eventingImpl.Invoke(Event{EventType: ProviderStale})
10061001

10071002
provider := struct {
10081003
FeatureProvider
@@ -1026,6 +1021,9 @@ func TestEventHandler_HandlersRunImmediately(t *testing.T) {
10261021
AddHandler(ProviderError, callback)
10271022
AddHandler(ProviderConfigChange, callback)
10281023

1024+
// Emit stale event from the provider.
1025+
eventingImpl.Invoke(Event{EventType: ProviderStale})
1026+
10291027
// assert client transitioned to STALE
10301028
eventually(t, func() bool {
10311029
return NewClient().State() == StaleState
@@ -1191,25 +1189,16 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
11911189

11921190
executor := newEventExecutor()
11931191

1194-
err := executor.registerDefaultProvider(eventingProvider)
1195-
if err != nil {
1196-
t.Fatal(err)
1197-
}
1192+
executor.registerDefaultProvider(eventingProvider)
11981193

11991194
if len(executor.activeSubscriptions) != 1 &&
12001195
executor.activeSubscriptions[0].featureProvider != eventingProvider.FeatureProvider {
12011196
t.Fatal("provider not added to active provider subscriptions")
12021197
}
12031198

1204-
err = executor.registerNamedEventingProvider("clientA", eventingProvider)
1205-
if err != nil {
1206-
t.Fatal(err)
1207-
}
1199+
executor.registerNamedEventingProvider("clientA", eventingProvider)
12081200

1209-
err = executor.registerNamedEventingProvider("clientB", eventingProvider)
1210-
if err != nil {
1211-
t.Fatal(err)
1212-
}
1201+
executor.registerNamedEventingProvider("clientB", eventingProvider)
12131202

12141203
if len(executor.activeSubscriptions) != 1 {
12151204
t.Fatal("multiple provided in active subscriptions")
@@ -1229,15 +1218,9 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
12291218

12301219
executor := newEventExecutor()
12311220

1232-
err := executor.registerDefaultProvider(eventingProvider)
1233-
if err != nil {
1234-
t.Fatal(err)
1235-
}
1221+
executor.registerDefaultProvider(eventingProvider)
12361222

1237-
err = executor.registerNamedEventingProvider("clientA", eventingProvider)
1238-
if err != nil {
1239-
t.Fatal(err)
1240-
}
1223+
executor.registerNamedEventingProvider("clientA", eventingProvider)
12411224

12421225
overridingProvider := struct {
12431226
FeatureProvider
@@ -1249,10 +1232,7 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
12491232
},
12501233
}
12511234

1252-
err = executor.registerNamedEventingProvider("clientA", overridingProvider)
1253-
if err != nil {
1254-
t.Fatal(err)
1255-
}
1235+
executor.registerNamedEventingProvider("clientA", overridingProvider)
12561236

12571237
if len(executor.activeSubscriptions) != 2 {
12581238
t.Fatal("error with active provider subscriptions")
@@ -1272,15 +1252,9 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
12721252

12731253
executor := newEventExecutor()
12741254

1275-
err := executor.registerNamedEventingProvider("clientA", eventingProvider)
1276-
if err != nil {
1277-
t.Fatal(err)
1278-
}
1255+
executor.registerNamedEventingProvider("clientA", eventingProvider)
12791256

1280-
err = executor.registerNamedEventingProvider("clientB", eventingProvider)
1281-
if err != nil {
1282-
t.Fatal(err)
1283-
}
1257+
executor.registerNamedEventingProvider("clientB", eventingProvider)
12841258

12851259
overridingProvider := struct {
12861260
FeatureProvider
@@ -1292,10 +1266,7 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
12921266
},
12931267
}
12941268

1295-
err = executor.registerNamedEventingProvider("clientA", overridingProvider)
1296-
if err != nil {
1297-
t.Fatal(err)
1298-
}
1269+
executor.registerNamedEventingProvider("clientA", overridingProvider)
12991270

13001271
if len(executor.activeSubscriptions) != 2 {
13011272
t.Fatal("error with active provider subscriptions")
@@ -1315,10 +1286,7 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
13151286

13161287
executor := newEventExecutor()
13171288

1318-
err := executor.registerNamedEventingProvider("clientA", eventingProvider)
1319-
if err != nil {
1320-
t.Fatal(err)
1321-
}
1289+
executor.registerNamedEventingProvider("clientA", eventingProvider)
13221290

13231291
overridingProvider := struct {
13241292
FeatureProvider
@@ -1330,10 +1298,7 @@ func TestEventHandler_1ToNMapping(t *testing.T) {
13301298
},
13311299
}
13321300

1333-
err = executor.registerNamedEventingProvider("clientA", overridingProvider)
1334-
if err != nil {
1335-
t.Fatal(err)
1336-
}
1301+
executor.registerNamedEventingProvider("clientA", overridingProvider)
13371302

13381303
if len(executor.activeSubscriptions) != 1 &&
13391304
executor.activeSubscriptions[0].featureProvider != overridingProvider.FeatureProvider {
@@ -1552,8 +1517,7 @@ func TestEventHandler_ChannelClosure(t *testing.T) {
15521517

15531518
executor := newEventExecutor()
15541519

1555-
err := executor.registerDefaultProvider(eventingProvider)
1556-
require.NoError(t, err)
1520+
executor.registerDefaultProvider(eventingProvider)
15571521
require.Len(t, executor.activeSubscriptions, 1)
15581522

15591523
var eventCount atomic.Int64

‎go.mod‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,8 @@ require (
66
github.com/cucumber/godog v0.15.1
77
github.com/stretchr/testify v1.11.1
88
go.uber.org/mock v0.6.0
9-
golang.org/x/sync v0.18.0
10-
golang.org/x/text v0.31.0
9+
golang.org/x/sync v0.19.0
10+
golang.org/x/text v0.33.0
1111
)
1212

1313
require (

‎go.sum‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,12 @@ go.uber.org/mock v0.6.0 h1:hyF9dfmbgIX5EfOdasqLsWD6xqpNZlXblLB/Dbnwv3Y=
5353
go.uber.org/mock v0.6.0/go.mod h1:KiVJ4BqZJaMj4svdfmHM0AUx4NJYO8ZNpPnZn1Z+BBU=
5454
golang.org/x/sync v0.18.0 h1:kr88TuHDroi+UVf+0hZnirlk8o8T+4MrK6mr60WkH/I=
5555
golang.org/x/sync v0.18.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI=
56+
golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4=
57+
golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI=
5658
golang.org/x/text v0.31.0 h1:aC8ghyu4JhP8VojJ2lEHBnochRno1sgL6nEi9WGFGMM=
5759
golang.org/x/text v0.31.0/go.mod h1:tKRAlv61yKIjGGHX/4tP1LTbc13YSec1pxVEWXzfoeM=
60+
golang.org/x/text v0.33.0 h1:B3njUFyqtHDUI5jMn1YIr5B0IE2U0qck04r6d4KPAxE=
61+
golang.org/x/text v0.33.0/go.mod h1:LuMebE6+rBincTi9+xWTY8TztLzKHc/9C1uBCG27+q8=
5862
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
5963
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
6064
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=

0 commit comments

Comments
 (0)