Skip to content

Commit a22b4a2

Browse files
toddbaerttoddbaert
committed
feat: provider wrapper to support state race fix
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
1 parent fb2ed4a commit a22b4a2

3 files changed

Lines changed: 120 additions & 55 deletions

File tree

‎packages/shared/src/open-feature.ts‎

Lines changed: 58 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type { BaseHook, EvaluationLifeCycle } from './hooks';
1616
import type { Logger, ManageLogger } from './logger';
1717
import { DefaultLogger, SafeLogger } from './logger';
1818
import type { ClientProviderStatus, CommonProvider, ProviderMetadata, ServerProviderStatus } from './provider';
19+
import { isStateManagingProvider } from './provider';
1920
import { objectOrUndefined, stringOrUndefined } from './type-guards';
2021
import type { Paradigm } from './types';
2122

@@ -27,28 +28,31 @@ type AnyProviderStatus = ClientProviderStatus | ServerProviderStatus;
2728
*/
2829
export class ProviderWrapper<P extends CommonProvider<AnyProviderStatus>, S extends AnyProviderStatus> {
2930
private _pendingContextChanges = 0;
31+
private readonly _delegateManagesState: boolean;
3032

3133
constructor(
3234
private _provider: P,
3335
private _status: S,
3436
_statusEnumType: typeof ClientProviderStatus | typeof ServerProviderStatus,
3537
) {
36-
// update the providers status with events
37-
_provider.events?.addHandler(AllProviderEvents.Ready, () => {
38-
// These casts are due to the face we don't "know" what status enum we are dealing with here (client or server).
39-
// We could abstract this an implement it in the client/server libs to fix this, but the value is low.
40-
this._status = _statusEnumType.READY as S;
41-
});
42-
_provider.events?.addHandler(AllProviderEvents.Stale, () => {
43-
this._status = _statusEnumType.STALE as S;
44-
});
45-
_provider.events?.addHandler(AllProviderEvents.Error, (details) => {
46-
if (details?.errorCode === ErrorCode.PROVIDER_FATAL) {
47-
this._status = _statusEnumType.FATAL as S;
48-
} else {
49-
this._status = _statusEnumType.ERROR as S;
50-
}
51-
});
38+
this._delegateManagesState = isStateManagingProvider(_provider);
39+
40+
// For legacy providers, update status from events. State-managing providers own their own status.
41+
if (!this._delegateManagesState) {
42+
_provider.events?.addHandler(AllProviderEvents.Ready, () => {
43+
this._status = _statusEnumType.READY as S;
44+
});
45+
_provider.events?.addHandler(AllProviderEvents.Stale, () => {
46+
this._status = _statusEnumType.STALE as S;
47+
});
48+
_provider.events?.addHandler(AllProviderEvents.Error, (details) => {
49+
if (details?.errorCode === ErrorCode.PROVIDER_FATAL) {
50+
this._status = _statusEnumType.FATAL as S;
51+
} else {
52+
this._status = _statusEnumType.ERROR as S;
53+
}
54+
});
55+
}
5256
}
5357

5458
get provider(): P {
@@ -60,11 +64,21 @@ export class ProviderWrapper<P extends CommonProvider<AnyProviderStatus>, S exte
6064
}
6165

6266
get status(): S {
67+
if (this._delegateManagesState) {
68+
return this._provider.status as S;
69+
}
6370
return this._status;
6471
}
6572

6673
set status(status: S) {
67-
this._status = status;
74+
// No-op for state-managing providers — they own their own status.
75+
if (!this._delegateManagesState) {
76+
this._status = status;
77+
}
78+
}
79+
80+
get delegateManagesState(): boolean {
81+
return this._delegateManagesState;
6882
}
6983

7084
get allContextChangesSettled() {
@@ -256,11 +270,14 @@ export abstract class OpenFeatureCommonAPI<
256270
.initialize?.(domain ? (this._domainScopedContext.get(domain) ?? this._context) : this._context)
257271
?.then(() => {
258272
wrappedProvider.status = this._statusEnumType.READY;
259-
// fetch the most recent event emitters, some may have been added during init
260-
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
261-
emitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
262-
});
263-
this._apiEmitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
273+
// State-managing providers emit their own events; skip SDK-side emission.
274+
if (!wrappedProvider.delegateManagesState) {
275+
// fetch the most recent event emitters, some may have been added during init
276+
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
277+
emitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
278+
});
279+
this._apiEmitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
280+
}
264281
})
265282
?.catch((error) => {
266283
// if this is a fatal error, transition to FATAL status
@@ -269,29 +286,35 @@ export abstract class OpenFeatureCommonAPI<
269286
} else {
270287
wrappedProvider.status = this._statusEnumType.ERROR;
271288
}
272-
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
273-
emitter?.emit(AllProviderEvents.Error, {
289+
// State-managing providers emit their own events; skip SDK-side emission.
290+
if (!wrappedProvider.delegateManagesState) {
291+
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
292+
emitter?.emit(AllProviderEvents.Error, {
293+
clientName: domain,
294+
domain,
295+
providerName,
296+
message: error?.message,
297+
});
298+
});
299+
this._apiEmitter?.emit(AllProviderEvents.Error, {
274300
clientName: domain,
275301
domain,
276302
providerName,
277303
message: error?.message,
278304
});
279-
});
280-
this._apiEmitter?.emit(AllProviderEvents.Error, {
281-
clientName: domain,
282-
domain,
283-
providerName,
284-
message: error?.message,
285-
});
305+
}
286306
// rethrow after emitting error events, so that public methods can control error handling
287307
throw error;
288308
});
289309
} else {
290310
wrappedProvider.status = this._statusEnumType.READY;
291-
emitters.forEach((emitter) => {
292-
emitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
293-
});
294-
this._apiEmitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
311+
// State-managing providers emit their own events; skip SDK-side emission.
312+
if (!wrappedProvider.delegateManagesState) {
313+
emitters.forEach((emitter) => {
314+
emitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
315+
});
316+
this._apiEmitter?.emit(AllProviderEvents.Ready, { clientName: domain, domain, providerName });
317+
}
295318
}
296319

297320
if (domain) {

‎packages/shared/src/provider/provider.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,37 @@ export interface CommonProvider<S extends ClientProviderStatus | ServerProviderS
135135
*/
136136
track?(trackingEventName: string, context: EvaluationContext, trackingEventDetails: TrackingEventDetails): void;
137137
}
138+
139+
/**
140+
* A provider that manages its own state. The SDK reads state from the provider
141+
* rather than maintaining shadow state. Implementations MUST ensure that `status`
142+
* is safe for concurrent access and that state transitions and associated event
143+
* emissions are atomic from the perspective of external observers.
144+
*
145+
* Legacy providers that do not implement this interface continue to have their state
146+
* managed by the SDK (deprecated behavior, to be removed in the next major version).
147+
*/
148+
export interface StateManagingProvider<
149+
S extends ClientProviderStatus | ServerProviderStatus,
150+
> extends CommonProvider<S> {
151+
/**
152+
* The current state of this provider. Must reflect NOT_READY before initialize()
153+
* is called and after onClose() completes. Must reflect READY if initialize()
154+
* resolves successfully.
155+
*/
156+
readonly status: S;
157+
158+
/**
159+
* Discriminant indicating that this provider manages its own state.
160+
*/
161+
readonly managesState: true;
162+
}
163+
164+
/**
165+
* Type guard for providers that manage their own state.
166+
*/
167+
export function isStateManagingProvider<S extends ClientProviderStatus | ServerProviderStatus>(
168+
provider: CommonProvider<S>,
169+
): provider is StateManagingProvider<S> {
170+
return 'managesState' in provider && (provider as StateManagingProvider<S>).managesState === true;
171+
}

‎packages/web/src/open-feature.ts‎

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,7 @@ export class OpenFeatureAPI
378378
): Promise<void> {
379379
// this should always be set according to the typings, but let's be defensive considering JS
380380
const providerName = wrapper.provider?.metadata?.name || 'unnamed-provider';
381+
const skipStateAndEvents = wrapper.delegateManagesState;
381382

382383
try {
383384
if (typeof wrapper.provider.onContextChange === 'function') {
@@ -386,36 +387,43 @@ export class OpenFeatureAPI
386387
// only reconcile if the onContextChange method returns a promise
387388
if (maybePromise && typeof maybePromise?.then === 'function') {
388389
wrapper.incrementPendingContextChanges();
389-
wrapper.status = this._statusEnumType.RECONCILING;
390-
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
391-
emitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
392-
});
393-
this._apiEmitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
390+
// State-managing providers own their own state and event emissions.
391+
if (!skipStateAndEvents) {
392+
wrapper.status = this._statusEnumType.RECONCILING;
393+
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
394+
emitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
395+
});
396+
this._apiEmitter?.emit(ProviderEvents.Reconciling, { domain, providerName });
397+
}
394398

395399
await maybePromise;
396400
wrapper.decrementPendingContextChanges();
397401
}
398402
}
399403
// only run the event handlers, and update the state if the onContextChange method succeeded
400-
wrapper.status = this._statusEnumType.READY;
401-
if (wrapper.allContextChangesSettled) {
402-
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
403-
emitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
404-
});
405-
this._apiEmitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
404+
if (!skipStateAndEvents) {
405+
wrapper.status = this._statusEnumType.READY;
406+
if (wrapper.allContextChangesSettled) {
407+
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
408+
emitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
409+
});
410+
this._apiEmitter?.emit(ProviderEvents.ContextChanged, { clientName: domain, domain, providerName });
411+
}
406412
}
407413
} catch (err) {
408414
// run error handlers instead
409415
wrapper.decrementPendingContextChanges();
410-
wrapper.status = this._statusEnumType.ERROR;
411-
if (wrapper.allContextChangesSettled) {
412-
const error = err as Error | undefined;
413-
const message = `Error running ${providerName}'s context change handler: ${error?.message}`;
414-
this._logger?.error(`${message}`, err);
415-
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
416-
emitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
417-
});
418-
this._apiEmitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
416+
if (!skipStateAndEvents) {
417+
wrapper.status = this._statusEnumType.ERROR;
418+
if (wrapper.allContextChangesSettled) {
419+
const error = err as Error | undefined;
420+
const message = `Error running ${providerName}'s context change handler: ${error?.message}`;
421+
this._logger?.error(`${message}`, err);
422+
this.getAssociatedEventEmitters(domain).forEach((emitter) => {
423+
emitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
424+
});
425+
this._apiEmitter?.emit(ProviderEvents.Error, { clientName: domain, domain, providerName, message });
426+
}
419427
}
420428
}
421429
}

0 commit comments

Comments
 (0)