diff --git a/.changeset/rotten-dragons-guess.md b/.changeset/rotten-dragons-guess.md new file mode 100644 index 0000000000..c35de83f6e --- /dev/null +++ b/.changeset/rotten-dragons-guess.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Fixed a bug where occasionally the initialization order of multiple modules consuming a single extension point could happen in the wrong order. diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 394359e486..8aafcb21aa 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -514,5 +514,21 @@ describe('DependencyGraph', () => { }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); }); + + it('awaits all producers', async () => { + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['a'] }, + 3: { consumes: ['a'] }, + }).parallelTopologicalTraversal(async id => { + // Delaying 2 should not make 3 run, wait for 1 too first + if (id === '2') { + await new Promise(resolve => setTimeout(resolve, 100)); + } + return id; + }), + ).resolves.toEqual(['1', '2', '3']); + }); }); }); diff --git a/packages/backend-app-api/src/lib/DependencyGraph.ts b/packages/backend-app-api/src/lib/DependencyGraph.ts index 86fad3fbf2..2f9d6f8829 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.ts @@ -188,12 +188,25 @@ export class DependencyGraph { fn: (value: T) => Promise, ): Promise { const allProvided = this.#allProvided; - const producedSoFar = new Set(); const waiting = new Set(this.#nodes.values()); const visited = new Set>(); const results = new Array(); let inFlight = 0; // Keep track of how many callbacks are in flight, so that we know if we got stuck + // This keeps track of a counter of how many providers there are still left + // to be visited for each depdendency. This needs to be a counter instead of + // a flag for the special case where there are several providers of a given + // value, even though there may be only one consumer of it. + const producedRemaining = new Map(); + for (const node of this.#nodes) { + for (const provided of node.provides) { + producedRemaining.set( + provided, + (producedRemaining.get(provided) ?? 0) + 1, + ); + } + } + // Find all nodes that have no dependencies that have not already been produced by visited nodes async function processMoreNodes() { if (waiting.size === 0) { @@ -203,7 +216,10 @@ export class DependencyGraph { for (const node of waiting) { let ready = true; for (const consumed of node.consumes) { - if (allProvided.has(consumed) && !producedSoFar.has(consumed)) { + if ( + allProvided.has(consumed) && + producedRemaining.get(consumed) !== 0 + ) { ready = false; continue; } @@ -234,7 +250,17 @@ export class DependencyGraph { const result = await fn(node.value); results.push(result); - node.provides.forEach(produced => producedSoFar.add(produced)); + node.provides.forEach(produced => { + const remaining = producedRemaining.get(produced); + if (!remaining) { + // This should be impossible, if the code that generates the map is correct + throw new Error( + `Internal error: Node provided superfluous dependency '${produced}'`, + ); + } + producedRemaining.set(produced, remaining - 1); + }); + inFlight -= 1; await processMoreNodes(); } diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index 3b0596cd13..9e8a6579fd 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -1134,4 +1134,68 @@ describe('BackendInitializer', () => { backend.add(instanceMetadataPlugin); await backend.start(); }); + + it('should properly wait for all modules that consume an extension point to really finish, before starting the module that provides that extension point', async () => { + expect.assertions(3); + const backend = new BackendInitializer(baseFactories); + const ext = createExtensionPoint<{ hello: (message: string) => void }>({ + id: 'a', + }); + const plugin = createBackendPlugin({ + pluginId: 'test', + register(reg) { + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + }); + const producerModule = createBackendModule({ + pluginId: 'test', + moduleId: 'producer', + register(reg) { + const hello = jest.fn(); + reg.registerExtensionPoint(ext, { hello }); + reg.registerInit({ + deps: {}, + async init() { + // we must not have been initialized before both of the consuming modules have been initialized + expect(hello).toHaveBeenCalledTimes(2); + expect(hello).toHaveBeenNthCalledWith(1, 'fast'); + expect(hello).toHaveBeenNthCalledWith(2, 'slow'); + }, + }); + }, + }); + const fastConsumerModule = createBackendModule({ + pluginId: 'test', + moduleId: 'fast-consumer', + register(reg) { + reg.registerInit({ + deps: { x: ext }, + async init({ x }) { + x.hello('fast'); + }, + }); + }, + }); + const slowConsumerModule = createBackendModule({ + pluginId: 'test', + moduleId: 'slow-consumer', + register(reg) { + reg.registerInit({ + deps: { x: ext }, + async init({ x }) { + await new Promise(resolve => setTimeout(resolve, 100)); + x.hello('slow'); + }, + }); + }, + }); + await backend.add(plugin); + await backend.add(producerModule); + await backend.add(fastConsumerModule); + await backend.add(slowConsumerModule); + await backend.start(); + }); });