From 1dd1e17361ef85e89f858d00475830bffec4173b Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Mon, 26 Jun 2023 13:00:54 -0400 Subject: [PATCH] fix: various event handler issues * fixed an issue where event handlers could be lost when providers updated * fixed an issue where handlers that throw would inappropriately not be caught * lazily store event emitters to reduce memory usage Signed-off-by: Todd Baert Signed-off-by: Lukas Reining Co-authored-by: Lukas Reining Co-authored-by: Michael Beemer --- packages/client/src/client.ts | 16 ++++++++----- packages/client/src/open-feature.ts | 2 +- packages/client/test/events.spec.ts | 25 +++++++++++++++++-- packages/server/src/client.ts | 14 +++++++---- packages/server/src/open-feature.ts | 4 ++-- packages/server/test/events.spec.ts | 25 +++++++++++++++++-- packages/shared/src/open-feature.ts | 37 ++++++++++++++++++++++------- 7 files changed, 96 insertions(+), 27 deletions(-) diff --git a/packages/client/src/client.ts b/packages/client/src/client.ts index 38a978fc0..52a56d5e2 100644 --- a/packages/client/src/client.ts +++ b/packages/client/src/client.ts @@ -33,7 +33,7 @@ export class OpenFeatureClient implements Client { // functions are passed here to make sure that these values are always up to date, // and so we don't have to make these public properties on the API class. private readonly providerAccessor: () => Provider, - private readonly events: () => OpenFeatureEventEmitter, + private readonly emitterAccessor: () => OpenFeatureEventEmitter, private readonly globalLogger: () => Logger, private readonly options: OpenFeatureClientOptions ) {} @@ -47,21 +47,25 @@ export class OpenFeatureClient implements Client { } addHandler(eventType: ProviderEvents, handler: EventHandler): void { - this.events().addHandler(eventType, handler); + this.emitterAccessor().addHandler(eventType, handler); const providerReady = !this._provider.status || this._provider.status === ProviderStatus.READY; if (eventType === ProviderEvents.Ready && providerReady) { // run immediately, we're ready. - handler({ clientName: this.metadata.name }); + try { + handler({ clientName: this.metadata.name }); + } catch (err) { + this._logger?.error('Error running event handler:', err); + } } } - removeHandler(notificationType: ProviderEvents, handler: EventHandler) { - this.events().removeHandler(notificationType, handler); + removeHandler(notificationType: ProviderEvents, handler: EventHandler): void { + this.emitterAccessor().removeHandler(notificationType, handler); } getHandlers(eventType: ProviderEvents) { - return this.events().getHandlers(eventType); + return this.emitterAccessor().getHandlers(eventType); } setLogger(logger: Logger): OpenFeatureClient { diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts index ae6c26460..3dd7a482d 100644 --- a/packages/client/src/open-feature.ts +++ b/packages/client/src/open-feature.ts @@ -77,7 +77,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI { // functions are passed here to make sure that these values are always up to date, // and so we don't have to make these public properties on the API class. () => this.getProviderForClient(name), - () => this.getEventEmitterForClient(name), + () => this.getAndCacheEventEmitterForClient(name), () => this._logger, { name, version } ); diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts index 2a350a0d0..364febe11 100644 --- a/packages/client/test/events.spec.ts +++ b/packages/client/test/events.spec.ts @@ -191,9 +191,30 @@ describe('Events', () => { OpenFeature.setProvider(clientId, provider); }); - }); - describe('Requirement 5.1.3', () => { + it('un-bound client event handlers still run after new provider set', (done) => { + const defaultProvider = new MockProvider({ name: 'default' }); + const namedProvider = new MockProvider(); + const unboundName = 'unboundName'; + const boundName = 'boundName'; + + // set the default provider + OpenFeature.setProvider(defaultProvider); + + // get a client using the default because it has not other mapping + const unBoundClient = OpenFeature.getClient(unboundName); + unBoundClient.addHandler(ProviderEvents.ConfigurationChanged, () => { + done(); + }); + + // get a client and assign a provider to it + OpenFeature.setProvider(boundName, namedProvider); + OpenFeature.getClient(boundName); + + // fire events + defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged); + }); + it('PROVIDER_ERROR events populates the message field', (done) => { const provider = new MockProvider({ failOnInit: true }); const client = OpenFeature.getClient(clientId); diff --git a/packages/server/src/client.ts b/packages/server/src/client.ts index 07050ff84..98009c837 100644 --- a/packages/server/src/client.ts +++ b/packages/server/src/client.ts @@ -34,7 +34,7 @@ export class OpenFeatureClient implements Client { // we always want the client to use the current provider, // so pass a function to always access the currently registered one. private readonly providerAccessor: () => Provider, - private readonly events: () => OpenFeatureEventEmitter, + private readonly emitterAccessor: () => OpenFeatureEventEmitter, private readonly globalLogger: () => Logger, private readonly options: OpenFeatureClientOptions, context: EvaluationContext = {} @@ -51,21 +51,25 @@ export class OpenFeatureClient implements Client { } addHandler(eventType: ProviderEvents, handler: EventHandler): void { - this.events().addHandler(eventType, handler); + this.emitterAccessor().addHandler(eventType, handler); const providerReady = !this._provider.status || this._provider.status === ProviderStatus.READY; if (eventType === ProviderEvents.Ready && providerReady) { // run immediately, we're ready. - handler({ clientName: this.metadata.name }); + try { + handler({ clientName: this.metadata.name }); + } catch (err) { + this._logger?.error('Error running event handler:', err); + } } } removeHandler(eventType: ProviderEvents, handler: EventHandler) { - this.events().removeHandler(eventType, handler); + this.emitterAccessor().removeHandler(eventType, handler); } getHandlers(eventType: ProviderEvents) { - return this.events().getHandlers(eventType); + return this.emitterAccessor().getHandlers(eventType); } setLogger(logger: Logger): OpenFeatureClient { diff --git a/packages/server/src/open-feature.ts b/packages/server/src/open-feature.ts index cfc9d4cdb..a8015a879 100644 --- a/packages/server/src/open-feature.ts +++ b/packages/server/src/open-feature.ts @@ -105,8 +105,8 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI { objectOrUndefined(contextOrUndefined); return new OpenFeatureClient( - () => this.getProviderForClient.bind(this)(name), - () => this.getEventEmitterForClient(name), + () => this.getProviderForClient(name), + () => this.getAndCacheEventEmitterForClient(name), () => this._logger, { name, version }, context diff --git a/packages/server/test/events.spec.ts b/packages/server/test/events.spec.ts index bb4d11eaf..691d6aa43 100644 --- a/packages/server/test/events.spec.ts +++ b/packages/server/test/events.spec.ts @@ -194,9 +194,30 @@ describe('Events', () => { OpenFeature.setProvider(clientId, provider); }); - }); - describe('Requirement 5.1.3', () => { + it('un-bound client event handlers still run after new provider set', (done) => { + const defaultProvider = new MockProvider({ name: 'default' }); + const namedProvider = new MockProvider(); + const unboundName = 'unboundName'; + const boundName = 'boundName'; + + // set the default provider + OpenFeature.setProvider(defaultProvider); + + // get a client using the default because it has not other mapping + const unBoundClient = OpenFeature.getClient(unboundName); + unBoundClient.addHandler(ProviderEvents.ConfigurationChanged, () => { + done(); + }); + + // get a client and assign a provider to it + OpenFeature.setProvider(boundName, namedProvider); + OpenFeature.getClient(boundName); + + // fire events + defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged); + }); + it('PROVIDER_ERROR events populates the message field', (done) => { const provider = new MockProvider({ failOnInit: true }); const client = OpenFeature.getClient(clientId); diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts index c33a4d61e..a414423c7 100644 --- a/packages/shared/src/open-feature.ts +++ b/packages/shared/src/open-feature.ts @@ -19,6 +19,7 @@ export abstract class OpenFeatureCommonAPI

this._logger); + private readonly _clientEventHandlers: Map = new Map(); protected _clientProviders: Map = new Map(); protected _clientEvents: Map = new Map(); @@ -103,7 +104,7 @@ export abstract class OpenFeatureCommonAPI

this._logger); this._clientEvents.set(name, newEmitter); + + const clientProvider = this.getProviderForClient(name); + Object.values(ProviderEvents).forEach((eventType) => + clientProvider.events?.addHandler(eventType, async (details?: EventDetails) => { + newEmitter.emit(eventType, { ...details, clientName: name }); + }) + ); + return newEmitter; } @@ -163,16 +173,25 @@ export abstract class OpenFeatureCommonAPI

oldProvider.events?.removeHandler(...eventHandler)); // iterate over the event types - Object.values(ProviderEvents).forEach((eventType) => - newProvider.events?.addHandler(eventType, async (details?: EventDetails) => { - // on each event type, fire the associated handlers - clientEmitter.emit(eventType, { ...details, clientName }); - this._events.emit(eventType, { ...details, clientName }); - }) + const newClientHandlers = Object.values(ProviderEvents).map<[ProviderEvents, EventHandler]>( + (eventType) => { + const handler = async (details?: EventDetails) => { + // on each event type, fire the associated handlers + clientEmitter.emit(eventType, { ...details, clientName }); + this._events.emit(eventType, { ...details, clientName }); + }; + + return [eventType, handler]; + } ); + + this._clientEventHandlers.set(clientName, newClientHandlers); + newClientHandlers.forEach((eventHandler) => newProvider.events?.addHandler(...eventHandler)); } async close(): Promise {