From 1338cca2f0f5906ced8c1554198f02a2ef8edaa1 Mon Sep 17 00:00:00 2001
From: Patrik Oldsberg <poldsberg@gmail.com>
Date: Tue, 30 Aug 2022 15:56:41 +0200
Subject: [PATCH] backend-app-api: fix ServiceRegistry default factory loader
 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        | 23 +++++++++++++++++++
 .../src/wiring/ServiceRegistry.ts             |  7 +++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
index f66e430c40..d5b688c0de 100644
--- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
+++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts
@@ -17,6 +17,7 @@
 import {
   createServiceRef,
   createServiceFactory,
+  ServiceRef,
 } from '@backstage/backend-plugin-api';
 import { ServiceRegistry } from './ServiceRegistry';
 
@@ -171,6 +172,28 @@ describe('ServiceRegistry', () => {
     expect(await factoryA('catalog')).not.toBe(await factoryB('catalog'));
   });
 
+  it('should only call each default factory loader once', async () => {
+    const factoryLoader = jest.fn(async (service: ServiceRef<void>) =>
+      createServiceFactory({
+        service,
+        deps: {},
+        factory: async () => async () => {},
+      }),
+    );
+    const ref = createServiceRef<void>({
+      id: '1',
+      defaultFactory: factoryLoader,
+    });
+
+    const registry = new ServiceRegistry([]);
+    const factory = registry.get(ref)!;
+    await Promise.all([
+      expect(factory('catalog')).resolves.toBeUndefined(),
+      expect(factory('catalog')).resolves.toBeUndefined(),
+    ]);
+    expect(factoryLoader).toHaveBeenCalledTimes(1);
+  });
+
   it('should not call factory functions more than once', async () => {
     const innerFactory = jest.fn(async (pluginId: string) => {
       return { x: 1, pluginId };
diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts
index c99e92d761..9df092db42 100644
--- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts
+++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts
@@ -21,7 +21,7 @@ import {
 
 export class ServiceRegistry {
   readonly #providedFactories: Map<string, ServiceFactory>;
-  readonly #loadedDefaultFactories: Map<Function, ServiceFactory>;
+  readonly #loadedDefaultFactories: Map<Function, Promise<ServiceFactory>>;
   readonly #implementations: Map<
     ServiceFactory,
     {
@@ -47,10 +47,11 @@ export class ServiceRegistry {
       if (!factory) {
         let loadedFactory = this.#loadedDefaultFactories.get(defaultFactory!);
         if (!loadedFactory) {
-          loadedFactory = (await defaultFactory!(ref)) as ServiceFactory;
+          loadedFactory = defaultFactory!(ref) as Promise<ServiceFactory>;
           this.#loadedDefaultFactories.set(defaultFactory!, loadedFactory);
         }
-        factory = loadedFactory;
+        // NOTE: This await is safe as long as #providedFactories is not mutated.
+        factory = await loadedFactory;
       }
 
       let implementation = this.#implementations.get(factory);