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 <[email protected]> Signed-off-by: Lukas Reining <[email protected]> Co-authored-by: Lukas Reining <[email protected]> Co-authored-by: Michael Beemer <[email protected]>
open-feature · Jun 26, 2023 · 1dd1e17 · 1dd1e17
1 parent b490cde
commit 1dd1e17
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 27 deletions.
diff --git 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
@@ -77,7 +77,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> {
       // 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
@@ -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
@@ -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
@@ -105,8 +105,8 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> {
       objectOrUndefined<EvaluationContext>(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
@@ -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
@@ -19,6 +19,7 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
   protected abstract _defaultProvider: P;
 
   private readonly _events = new OpenFeatureEventEmitter(() => this._logger);
+  private readonly _clientEventHandlers: Map<string | undefined, [ProviderEvents, EventHandler][]> = new Map();
   protected _clientProviders: Map<string, P> = new Map();
   protected _clientEvents: Map<string | undefined, OpenFeatureEventEmitter> = new Map();
 
@@ -103,7 +104,7 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
       return this;
     }
 
-    const clientEmitter = this.getEventEmitterForClient(clientName);
+    const clientEmitter = this.getAndCacheEventEmitterForClient(clientName);
 
     if (typeof provider.initialize === 'function') {
       provider
@@ -145,15 +146,24 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
     return this._clientProviders.get(name) ?? this._defaultProvider;
   }
 
-  protected getEventEmitterForClient(name?: string): OpenFeatureEventEmitter {
+  protected getAndCacheEventEmitterForClient(name?: string): OpenFeatureEventEmitter {
     const emitter = this._clientEvents.get(name);
 
     if (emitter) {
       return emitter;
     }
 
+    // lazily add the event emitters
     const newEmitter = new OpenFeatureEventEmitter(() => this._logger);
     this._clientEvents.set(name, newEmitter);
+
+    const clientProvider = this.getProviderForClient(name);
+    Object.values<ProviderEvents>(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<P extends CommonProvider = CommonProv
     clientName: string | undefined,
     clientEmitter: OpenFeatureEventEmitter
   ) {
-    oldProvider.events?.removeAllHandlers();
+    this._clientEventHandlers
+      .get(clientName)
+      ?.forEach((eventHandler) => oldProvider.events?.removeHandler(...eventHandler));
 
     // iterate over the event types
-    Object.values<ProviderEvents>(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>(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<void> {