Repository: open-feature/js-sdk
Pull request #501 (2023-07-22 05:21:20)

fix: race adding handler during init
Fixes a race condition that prevents handlers added while their provider is initializing from running.


Patch:
diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts
index a6cb4b31..675fc53d 100644
--- a/packages/client/src/open-feature.ts
+++ b/packages/client/src/open-feature.ts
@@ -60,7 +60,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> implements Ma
       // 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.getAndCacheEventEmitterForClient(name),
+      () => this.buildAndCacheEventEmitterForClient(name),
       () => this._logger,
       { name, version }
     );
diff --git a/packages/server/src/open-feature.ts b/packages/server/src/open-feature.ts
index 55e24c50..f426088e 100644
--- a/packages/server/src/open-feature.ts
+++ b/packages/server/src/open-feature.ts
@@ -98,7 +98,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> implements Ma
 
     return new OpenFeatureClient(
       () => this.getProviderForClient(name),
-      () => this.getAndCacheEventEmitterForClient(name),
+      () => this.buildAndCacheEventEmitterForClient(name),
       () => this._logger,
       { name, version },
       context
diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts
index da690eb1..d4c07761 100644
--- a/packages/shared/src/open-feature.ts
+++ b/packages/shared/src/open-feature.ts
@@ -122,20 +122,20 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
       return this;
     }
 
-    // get the named emitter, or if this is the default provider, get all event emitters not associated with a provider
-    const emitters = clientName ? [this.getAndCacheEventEmitterForClient(clientName)] : this.getUnboundEmitters();
+    const emitters = this.getAssociatedEventEmitters(clientName);
 
     if (typeof provider.initialize === 'function') {
       provider
         .initialize?.(this._context)
         ?.then(() => {
-          emitters.forEach((emitter) => {
+          // fetch the most recent event emitters, some may have been added during init
+          this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
             emitter?.emit(ProviderEvents.Ready, { clientName });
           });
           this._events?.emit(ProviderEvents.Ready, { clientName });
         })
         ?.catch((error) => {
-          emitters.forEach((emitter) => {
+          this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
             emitter?.emit(ProviderEvents.Error, { clientName, message: error.message });
           });
           this._events?.emit(ProviderEvents.Error, { clientName, message: error.message });
@@ -171,7 +171,7 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
     return this._clientProviders.get(name) ?? this._defaultProvider;
   }
 
-  protected getAndCacheEventEmitterForClient(name?: string): InternalEventEmitter {
+  protected buildAndCacheEventEmitterForClient(name?: string): InternalEventEmitter {
     const emitter = this._clientEvents.get(name);
 
     if (emitter) {
@@ -204,6 +204,10 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
     ].filter(isDefined);
   }
 
+  private getAssociatedEventEmitters(clientName: string | undefined) {
+    return clientName ? [this.buildAndCacheEventEmitterForClient(clientName)] : this.getUnboundEmitters();
+  }
+
   private transferListeners(
     oldProvider: P,
     newProvider: P,


Test patch:
diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts
index fae2813e..83082eb6 100644
--- a/packages/client/test/events.spec.ts
+++ b/packages/client/test/events.spec.ts
@@ -12,17 +12,21 @@ import {
 } from '../src';
 import { v4 as uuid } from 'uuid';
 
+const TIMEOUT = 1000;
+
 class MockProvider implements Provider {
   readonly metadata: ProviderMetadata;
   readonly events?: OpenFeatureEventEmitter;
   private hasInitialize: boolean;
   private failOnInit: boolean;
+  private initDelay?: number;
   private enableEvents: boolean;
   status?: ProviderStatus = undefined;
 
   constructor(options?: {
     hasInitialize?: boolean;
     initialStatus?: ProviderStatus;
+    initDelay?: number;
     enableEvents?: boolean;
     failOnInit?: boolean;
     name?: string;
@@ -30,6 +34,7 @@ class MockProvider implements Provider {
     this.metadata = { name: options?.name ?? 'mock-provider' };
     this.hasInitialize = options?.hasInitialize ?? true;
     this.status = options?.initialStatus ?? ProviderStatus.NOT_READY;
+    this.initDelay = options?.initDelay ?? 0;
     this.enableEvents = options?.enableEvents ?? true;
     this.failOnInit = options?.failOnInit ?? false;
 
@@ -39,6 +44,7 @@ class MockProvider implements Provider {
 
     if (this.hasInitialize) {
       this.initialize = jest.fn(async () => {
+        await new Promise((resolve) => setTimeout(resolve, this.initDelay));
         if (this.failOnInit) {
           throw new Error('Provider initialization failed');
         }
@@ -69,13 +75,15 @@ class MockProvider implements Provider {
 
 describe('Events', () => {
   // set timeouts short for this suite.
-  jest.setTimeout(1000);
+  jest.setTimeout(TIMEOUT);
   let clientId = uuid();
 
   afterEach(() => {
     jest.clearAllMocks();
     clientId = uuid();
-  });
+    // hacky, but it's helpful to clear the handlers between tests
+    (OpenFeature as any)._clientEventHandlers = new Map();
+    (OpenFeature as any)._clientEvents = new Map();  });
 
   beforeEach(() => {
     OpenFeature.setProvider(NOOP_PROVIDER);
@@ -296,6 +304,19 @@ describe('Events', () => {
       defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged);
     });
 
+    it('handler added while while provider initializing runs', (done) => {
+      const provider = new MockProvider({ name: 'race', initialStatus: ProviderStatus.NOT_READY, initDelay: TIMEOUT / 2 });
+
+      // set the default provider
+      OpenFeature.setProvider(provider);
+      const client = OpenFeature.getClient();
+
+      // add a handler while the provider is starting
+      client.addHandler(ProviderEvents.Ready, () => {
+        done();
+      });
+    });
+
     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/test/events.spec.ts b/packages/server/test/events.spec.ts
index 25e20cac..340691e1 100644
--- a/packages/server/test/events.spec.ts
+++ b/packages/server/test/events.spec.ts
@@ -12,17 +12,21 @@ import {
 } from '../src';
 import { v4 as uuid } from 'uuid';
 
+const TIMEOUT = 1000;
+
 class MockProvider implements Provider {
   readonly metadata: ProviderMetadata;
   readonly events?: OpenFeatureEventEmitter;
   private hasInitialize: boolean;
   private failOnInit: boolean;
+  private initDelay?: number;
   private enableEvents: boolean;
   status?: ProviderStatus = undefined;
 
   constructor(options?: {
     hasInitialize?: boolean;
     initialStatus?: ProviderStatus;
+    initDelay?: number;
     enableEvents?: boolean;
     failOnInit?: boolean;
     name?: string;
@@ -30,6 +34,7 @@ class MockProvider implements Provider {
     this.metadata = { name: options?.name ?? 'mock-provider' };
     this.hasInitialize = options?.hasInitialize ?? true;
     this.status = options?.initialStatus ?? ProviderStatus.NOT_READY;
+    this.initDelay = options?.initDelay ?? 0;
     this.enableEvents = options?.enableEvents ?? true;
     this.failOnInit = options?.failOnInit ?? false;
 
@@ -39,6 +44,7 @@ class MockProvider implements Provider {
 
     if (this.hasInitialize) {
       this.initialize = jest.fn(async () => {
+        await new Promise((resolve) => setTimeout(resolve, this.initDelay));
         if (this.failOnInit) {
           throw new Error('Provider initialization failed');
         }
@@ -72,12 +78,15 @@ class MockProvider implements Provider {
 
 describe('Events', () => {
   // set timeouts short for this suite.
-  jest.setTimeout(1000);
+  jest.setTimeout(TIMEOUT);
   let clientId = uuid();
 
   afterEach(() => {
     jest.clearAllMocks();
     clientId = uuid();
+    // hacky, but it's helpful to clear the handlers between tests
+    (OpenFeature as any)._clientEventHandlers = new Map();
+    (OpenFeature as any)._clientEvents = new Map();
   });
 
   beforeEach(() => {
@@ -299,6 +308,19 @@ describe('Events', () => {
       defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged);
     });
 
+    it('handler added while while provider initializing runs', (done) => {
+      const provider = new MockProvider({ name: 'race', initialStatus: ProviderStatus.NOT_READY, initDelay: TIMEOUT / 2 });
+
+      // set the default provider
+      OpenFeature.setProvider(provider);
+      const client = OpenFeature.getClient();
+
+      // add a handler while the provider is starting
+      client.addHandler(ProviderEvents.Ready, () => {
+        done();
+      });
+    });
+
     it('PROVIDER_ERROR events populates the message field', (done) => {
       const provider = new MockProvider({ failOnInit: true });
       const client = OpenFeature.getClient(clientId);