From 405d485788664afb4892e3b5c89d97d71b9b8e7f Mon Sep 17 00:00:00 2001
From: Patrik Oldsberg <poldsberg@gmail.com>
Date: Tue, 30 Aug 2022 15:39:27 +0200
Subject: [PATCH] backend-app-api: fix ServiceRegistry instantiation race
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Fredrik Adelöw <freben@gmail.com>
Co-authored-by: blam <ben@blam.sh>
Co-authored-by: Johan Haals <johan.haals@gmail.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Signed-off-by: Johan Haals <johan.haals@gmail.com>
---
 .../src/wiring/ServiceRegistry.test.ts        | 27 +++++++++++
 .../src/wiring/ServiceRegistry.ts             | 48 +++++++++++--------
 2 files changed, 55 insertions(+), 20 deletions(-)

diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
index acdd2a4c41..f66e430c40 100644
--- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
+++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
@@ -170,4 +170,31 @@ describe('ServiceRegistry', () => {
     expect(await factoryB('catalog')).toBe(await factoryB('catalog'));
     expect(await factoryA('catalog')).not.toBe(await factoryB('catalog'));
   });
+
+  it('should not call factory functions more than once', async () => {
+    const innerFactory = jest.fn(async (pluginId: string) => {
+      return { x: 1, pluginId };
+    });
+    const factory = jest.fn(async () => innerFactory);
+    const myFactory = createServiceFactory({
+      service: ref1,
+      deps: {},
+      factory,
+    });
+
+    const registry = new ServiceRegistry([myFactory]);
+
+    await Promise.all([
+      registry.get(ref1)!('catalog')!,
+      registry.get(ref1)!('catalog')!,
+      registry.get(ref1)!('catalog')!,
+      registry.get(ref1)!('scaffolder')!,
+      registry.get(ref1)!('scaffolder')!,
+    ]);
+
+    expect(factory).toHaveBeenCalledTimes(1);
+    expect(innerFactory).toHaveBeenCalledTimes(2);
+    expect(innerFactory).toHaveBeenCalledWith('catalog');
+    expect(innerFactory).toHaveBeenCalledWith('scaffolder');
+  });
 });
diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts
index b471cd67ca..c99e92d761 100644
--- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts
+++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts
@@ -22,7 +22,13 @@ import {
 export class ServiceRegistry {
   readonly #providedFactories: Map<string, ServiceFactory>;
   readonly #loadedDefaultFactories: Map<Function, ServiceFactory>;
-  readonly #implementations: Map<ServiceFactory, Map<string, unknown>>;
+  readonly #implementations: Map<
+    ServiceFactory,
+    {
+      factoryFunc: Promise<FactoryFunc<unknown>>;
+      byPlugin: Map<string, Promise<unknown>>;
+    }
+  >;
 
   constructor(factories: ServiceFactory<any>[]) {
     this.#providedFactories = new Map(factories.map(f => [f.service.id, f]));
@@ -47,29 +53,31 @@ export class ServiceRegistry {
         factory = loadedFactory;
       }
 
-      let implementations = this.#implementations.get(factory);
-      if (implementations) {
-        if (implementations.has(pluginId)) {
-          return implementations.get(pluginId) as T;
-        }
-      } else {
-        implementations = new Map();
-        this.#implementations.set(factory, implementations);
+      let implementation = this.#implementations.get(factory);
+      if (!implementation) {
+        const factoryDeps = Object.fromEntries(
+          Object.entries(factory.deps).map(([name, serviceRef]) => [
+            name,
+            this.get(serviceRef)!, // TODO: throw
+          ]),
+        );
+
+        implementation = {
+          factoryFunc: factory.factory(factoryDeps),
+          byPlugin: new Map(),
+        };
+
+        this.#implementations.set(factory, implementation);
       }
 
-      const factoryDeps = Object.fromEntries(
-        Object.entries(factory.deps).map(([name, serviceRef]) => [
-          name,
-          this.get(serviceRef)!, // TODO: throw
-        ]),
-      );
+      let result = implementation.byPlugin.get(pluginId) as Promise<any>;
+      if (!result) {
+        result = implementation.factoryFunc.then(func => func(pluginId));
 
-      const factoryFunc = await factory.factory(factoryDeps);
-      const implementation = await factoryFunc(pluginId);
+        implementation.byPlugin.set(pluginId, result);
+      }
 
-      implementations.set(pluginId, implementation);
-
-      return implementation as T;
+      return result;
     };
   }
 }