From c4e8fefd9f1383dd1e4b2e3125188ab19b94886c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 May 2023 15:44:08 +0200 Subject: [PATCH 01/12] errors: added ServiceUnavailableError Signed-off-by: Patrik Oldsberg --- .changeset/ninety-jeans-scream.md | 5 +++++ .changeset/tasty-oranges-sleep.md | 5 +++++ .../backend-app-api/src/http/MiddlewareFactory.test.ts | 8 ++++++++ packages/backend-app-api/src/http/MiddlewareFactory.ts | 3 +++ packages/errors/api-report.md | 3 +++ packages/errors/src/errors/common.ts | 7 +++++++ packages/errors/src/errors/index.ts | 1 + 7 files changed, 32 insertions(+) create mode 100644 .changeset/ninety-jeans-scream.md create mode 100644 .changeset/tasty-oranges-sleep.md diff --git a/.changeset/ninety-jeans-scream.md b/.changeset/ninety-jeans-scream.md new file mode 100644 index 0000000000..3465a67444 --- /dev/null +++ b/.changeset/ninety-jeans-scream.md @@ -0,0 +1,5 @@ +--- +'@backstage/errors': minor +--- + +Added `ServiceUnavailableError` diff --git a/.changeset/tasty-oranges-sleep.md b/.changeset/tasty-oranges-sleep.md new file mode 100644 index 0000000000..7c99ecc822 --- /dev/null +++ b/.changeset/tasty-oranges-sleep.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Added handling of `ServiceUnavailableError` to error handling middleware. diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts index 64812564ca..5dbb63ec59 100644 --- a/packages/backend-app-api/src/http/MiddlewareFactory.test.ts +++ b/packages/backend-app-api/src/http/MiddlewareFactory.test.ts @@ -21,6 +21,7 @@ import { NotAllowedError, NotFoundError, NotModifiedError, + ServiceUnavailableError, } from '@backstage/errors'; import express from 'express'; import createError from 'http-errors'; @@ -143,6 +144,9 @@ describe('MiddlewareFactory', () => { app.use('/ConflictError', () => { throw new ConflictError(); }); + app.use('/ServiceUnavailableError', () => { + throw new ServiceUnavailableError(); + }); app.use(middleware.error()); const r = request(app); @@ -165,6 +169,10 @@ describe('MiddlewareFactory', () => { expect((await r.get('/ConflictError')).body.error.name).toBe( 'ConflictError', ); + expect((await r.get('/ServiceUnavailableError')).status).toBe(503); + expect((await r.get('/ServiceUnavailableError')).body.error.name).toBe( + 'ServiceUnavailableError', + ); }); it('logs all 500 errors', async () => { diff --git a/packages/backend-app-api/src/http/MiddlewareFactory.ts b/packages/backend-app-api/src/http/MiddlewareFactory.ts index 833bdba540..774e784d1d 100644 --- a/packages/backend-app-api/src/http/MiddlewareFactory.ts +++ b/packages/backend-app-api/src/http/MiddlewareFactory.ts @@ -36,6 +36,7 @@ import { NotAllowedError, NotFoundError, NotModifiedError, + ServiceUnavailableError, serializeError, } from '@backstage/errors'; import { NotImplementedError } from '@backstage/errors'; @@ -260,6 +261,8 @@ function getStatusCode(error: Error): number { return 409; case NotImplementedError.name: return 501; + case ServiceUnavailableError.name: + return 503; default: break; } diff --git a/packages/errors/api-report.md b/packages/errors/api-report.md index 650a826a30..0d405d8ba6 100644 --- a/packages/errors/api-report.md +++ b/packages/errors/api-report.md @@ -125,6 +125,9 @@ export function serializeError( }, ): SerializedError; +// @public +export class ServiceUnavailableError extends CustomErrorBase {} + // @public export function stringifyError(error: unknown): string; ``` diff --git a/packages/errors/src/errors/common.ts b/packages/errors/src/errors/common.ts index e1a292d7b5..a2ea20e99f 100644 --- a/packages/errors/src/errors/common.ts +++ b/packages/errors/src/errors/common.ts @@ -81,6 +81,13 @@ export class NotModifiedError extends CustomErrorBase {} */ export class NotImplementedError extends CustomErrorBase {} +/** + * The server is not ready to handle the request. + * + * @public + */ +export class ServiceUnavailableError extends CustomErrorBase {} + /** * An error that forwards an underlying cause with additional context in the message. * diff --git a/packages/errors/src/errors/index.ts b/packages/errors/src/errors/index.ts index efd3257fbf..0a59891f87 100644 --- a/packages/errors/src/errors/index.ts +++ b/packages/errors/src/errors/index.ts @@ -25,6 +25,7 @@ export { NotFoundError, NotModifiedError, NotImplementedError, + ServiceUnavailableError, } from './common'; export { CustomErrorBase } from './CustomErrorBase'; export { ResponseError } from './ResponseError'; From 3bb4158a8aa43920b090cb2e8d2616528a15baf6 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 May 2023 15:55:26 +0200 Subject: [PATCH 02/12] backend-{plugin,app}-api: introduce startup hooks and parallelize initialization Signed-off-by: Patrik Oldsberg --- .changeset/ninety-turkeys-knock.md | 5 + .changeset/olive-carpets-explode.md | 5 + .../lifecycle/lifecycleServiceFactory.ts | 80 +++++++++-- .../rootLifecycleServiceFactory.ts | 42 +++++- .../src/wiring/BackendInitializer.ts | 132 ++++++++++-------- .../wiring/createSpecializedBackend.test.ts | 10 +- .../src/CreateBackend.test.ts | 15 +- packages/backend-plugin-api/api-report.md | 12 ++ .../services/definitions/LifecycleService.ts | 29 ++++ .../src/services/definitions/index.ts | 2 + 10 files changed, 256 insertions(+), 76 deletions(-) create mode 100644 .changeset/ninety-turkeys-knock.md create mode 100644 .changeset/olive-carpets-explode.md diff --git a/.changeset/ninety-turkeys-knock.md b/.changeset/ninety-turkeys-knock.md new file mode 100644 index 0000000000..7f26f0eeda --- /dev/null +++ b/.changeset/ninety-turkeys-knock.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Switched startup strategy to initialize all plugins in parallel, as well as hook into the new startup lifecycle hooks. diff --git a/.changeset/olive-carpets-explode.md b/.changeset/olive-carpets-explode.md new file mode 100644 index 0000000000..6741355d58 --- /dev/null +++ b/.changeset/olive-carpets-explode.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-plugin-api': patch +--- + +Added startup hooks to the lifecycle services. diff --git a/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts b/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts index 79658c0d42..c0cfcf7d8c 100644 --- a/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts @@ -14,12 +14,72 @@ * limitations under the License. */ import { - createServiceFactory, - coreServices, + LifecycleService, LifecycleServiceShutdownHook, LifecycleServiceShutdownOptions, + LifecycleServiceStartupHook, + LifecycleServiceStartupOptions, + LoggerService, + PluginMetadataService, + RootLifecycleService, + coreServices, + createServiceFactory, } from '@backstage/backend-plugin-api'; +/** @internal */ +export class BackendPluginLifecycleImpl implements LifecycleService { + constructor( + private readonly logger: LoggerService, + private readonly rootLifecycle: RootLifecycleService, + private readonly pluginMetadata: PluginMetadataService, + ) {} + + #hasStarted = false; + #startupTasks: Array<{ + hook: LifecycleServiceStartupHook; + options?: LifecycleServiceStartupOptions; + }> = []; + + addStartupHook( + hook: LifecycleServiceStartupHook, + options?: LifecycleServiceStartupOptions, + ): void { + this.#startupTasks.push({ hook, options }); + } + + async startup(): Promise { + if (this.#hasStarted) { + return; + } + this.#hasStarted = true; + + this.logger.debug( + `Running ${this.#startupTasks.length} plugin startup tasks...`, + ); + await Promise.all( + this.#startupTasks.map(async ({ hook, options }) => { + const logger = options?.logger ?? this.logger; + try { + await hook(); + logger.debug(`Plugin startup hook succeeded`); + } catch (error) { + logger.error(`Plugin startup hook failed, ${error}`); + } + }), + ); + } + + addShutdownHook( + hook: LifecycleServiceShutdownHook, + options?: LifecycleServiceShutdownOptions, + ): void { + const plugin = this.pluginMetadata.getId(); + this.rootLifecycle.addShutdownHook(hook, { + logger: options?.logger?.child({ plugin }) ?? this.logger, + }); + } +} + /** * Allows plugins to register shutdown hooks that are run when the process is about to exit. * @public @@ -32,16 +92,10 @@ export const lifecycleServiceFactory = createServiceFactory({ pluginMetadata: coreServices.pluginMetadata, }, async factory({ rootLifecycle, logger, pluginMetadata }) { - const plugin = pluginMetadata.getId(); - return { - addShutdownHook( - hook: LifecycleServiceShutdownHook, - options?: LifecycleServiceShutdownOptions, - ): void { - rootLifecycle.addShutdownHook(hook, { - logger: options?.logger?.child({ plugin }) ?? logger, - }); - }, - }; + return new BackendPluginLifecycleImpl( + logger, + rootLifecycle, + pluginMetadata, + ); }, }); diff --git a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts index f379c2ec52..045c5e8267 100644 --- a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts @@ -17,16 +17,52 @@ import { createServiceFactory, coreServices, + LifecycleServiceStartupHook, + LifecycleServiceStartupOptions, LifecycleServiceShutdownHook, LifecycleServiceShutdownOptions, RootLifecycleService, LoggerService, } from '@backstage/backend-plugin-api'; +/** @internal */ export class BackendLifecycleImpl implements RootLifecycleService { constructor(private readonly logger: LoggerService) {} - #isCalled = false; + #hasStarted = false; + #startupTasks: Array<{ + hook: LifecycleServiceStartupHook; + options?: LifecycleServiceStartupOptions; + }> = []; + + addStartupHook( + hook: LifecycleServiceStartupHook, + options?: LifecycleServiceStartupOptions, + ): void { + this.#startupTasks.push({ hook, options }); + } + + async startup(): Promise { + if (this.#hasStarted) { + return; + } + this.#hasStarted = true; + + this.logger.info(`Running ${this.#startupTasks.length} startup tasks...`); + await Promise.all( + this.#startupTasks.map(async ({ hook, options }) => { + const logger = options?.logger ?? this.logger; + try { + await hook(); + logger.info(`Startup hook succeeded`); + } catch (error) { + logger.error(`Startup hook failed, ${error}`); + } + }), + ); + } + + #hasShutdown = false; #shutdownTasks: Array<{ hook: LifecycleServiceShutdownHook; options?: LifecycleServiceShutdownOptions; @@ -40,10 +76,10 @@ export class BackendLifecycleImpl implements RootLifecycleService { } async shutdown(): Promise { - if (this.#isCalled) { + if (this.#hasShutdown) { return; } - this.#isCalled = true; + this.#hasShutdown = true; this.logger.info(`Running ${this.#shutdownTasks.length} shutdown tasks...`); await Promise.all( diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index b2c62c2122..a6d0e74866 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -21,6 +21,7 @@ import { ServiceRef, } from '@backstage/backend-plugin-api'; import { BackendLifecycleImpl } from '../services/implementations/rootLifecycle/rootLifecycleServiceFactory'; +import { BackendPluginLifecycleImpl } from '../services/implementations/lifecycle/lifecycleServiceFactory'; import { BackendRegisterInit, EnumerableServiceHolder, @@ -33,7 +34,8 @@ import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring export class BackendInitializer { #startPromise?: Promise; #features = new Array(); - #registerInits = new Array(); + #pluginInits = new Array(); + #moduleInits = new Map>(); #extensionPoints = new Map, unknown>(); #serviceHolder: EnumerableServiceHolder; @@ -130,7 +132,7 @@ export class BackendInitializer { } } - // Initialize all features + // Enumerate all features for (const feature of this.#features) { for (const r of feature.getRegistrations()) { const provides = new Set>(); @@ -147,24 +149,62 @@ export class BackendInitializer { } } - this.#registerInits.push({ - id: r.type === 'plugin' ? r.pluginId : `${r.pluginId}.${r.moduleId}`, - provides, - consumes: new Set(Object.values(r.init.deps)), - init: r.init, - }); + if (r.type === 'plugin') { + this.#pluginInits.push({ + id: r.pluginId, + provides, + consumes: new Set(Object.values(r.init.deps)), + init: r.init, + }); + } else { + let modules = this.#moduleInits.get(r.pluginId); + if (!modules) { + modules = []; + this.#moduleInits.set(r.pluginId, modules); + } + modules.push({ + id: `${r.pluginId}.${r.moduleId}`, + provides, + consumes: new Set(Object.values(r.init.deps)), + init: r.init, + }); + } } } - const orderedRegisterResults = this.#resolveInitOrder(this.#registerInits); + // All plugins are initialized in parallel + await Promise.all( + this.#pluginInits.map(async pluginInit => { + // Modules are initialized before plugins, so that they can provide extension to the plugin + const modules = this.#moduleInits.get(pluginInit.id) ?? []; + await Promise.all( + modules.map(async moduleInit => { + const moduleDeps = await this.#getInitDeps( + moduleInit.init.deps, + moduleInit.id, + ); + await moduleInit.init.func(moduleDeps); + }), + ); - for (const registerInit of orderedRegisterResults) { - const deps = await this.#getInitDeps( - registerInit.init.deps, - registerInit.id, - ); - await registerInit.init.func(deps); - } + // Once all modules have been initialized, we can initialize the plugin itself + const pluginDeps = await this.#getInitDeps( + pluginInit.init.deps, + pluginInit.id, + ); + await pluginInit.init.func(pluginDeps); + + // Once the plugin and all modules have been initialized, we can signal that the plugin has stared up successfully + const lifecycleService = await this.#getPluginLifecycleImpl( + pluginInit.id, + ); + await lifecycleService.startup(); + }), + ); + + // Once all plugins and modules have been initialized, we can signal that the backend has started up successfully + const lifecycleService = await this.#getRootLifecycleImpl(); + await lifecycleService.startup(); // Once the backend is started, any uncaught errors or unhandled rejections are caught // and logged, in order to avoid crashing the entire backend on local failures. @@ -186,56 +226,38 @@ export class BackendInitializer { } } - #resolveInitOrder(registerInits: Array) { - let registerInitsToOrder = registerInits.slice(); - const orderedRegisterInits = new Array(); - - // TODO: Validate duplicates - - while (registerInitsToOrder.length > 0) { - const toRemove = new Set(); - - for (const registerInit of registerInitsToOrder) { - const unInitializedDependents = []; - - for (const provided of registerInit.provides) { - if ( - registerInitsToOrder.some( - init => init !== registerInit && init.consumes.has(provided), - ) - ) { - unInitializedDependents.push(provided); - } - } - - if (unInitializedDependents.length === 0) { - orderedRegisterInits.push(registerInit); - toRemove.add(registerInit); - } - } - - registerInitsToOrder = registerInitsToOrder.filter(r => !toRemove.has(r)); - } - - return orderedRegisterInits; - } - async stop(): Promise { if (!this.#startPromise) { return; } await this.#startPromise; + const lifecycleService = await this.#getRootLifecycleImpl(); + await lifecycleService.shutdown(); + } + + // Bit of a hacky way to grab the lifecycle services, potentially find a nicer way to do this + async #getRootLifecycleImpl(): Promise { const lifecycleService = await this.#serviceHolder.get( coreServices.rootLifecycle, 'root', ); - - // TODO(Rugvip): Find a better way to do this if (lifecycleService instanceof BackendLifecycleImpl) { - await lifecycleService.shutdown(); - } else { - throw new Error('Unexpected lifecycle service implementation'); + return lifecycleService; } + throw new Error('Unexpected root lifecycle service implementation'); + } + + async #getPluginLifecycleImpl( + pluginId: string, + ): Promise { + const lifecycleService = await this.#serviceHolder.get( + coreServices.lifecycle, + pluginId, + ); + if (lifecycleService instanceof BackendPluginLifecycleImpl) { + return lifecycleService; + } + throw new Error('Unexpected plugin lifecycle service implementation'); } } diff --git a/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts b/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts index ad50455e5f..dc9a0cc085 100644 --- a/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts +++ b/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts @@ -32,12 +32,18 @@ describe('createSpecializedBackend', () => { createServiceFactory({ service: coreServices.rootLifecycle, deps: {}, - factory: async () => ({ addShutdownHook: () => {} }), + factory: async () => ({ + addStartupHook: () => {}, + addShutdownHook: () => {}, + }), }), createServiceFactory({ service: coreServices.rootLifecycle, deps: {}, - factory: async () => ({ addShutdownHook: () => {} }), + factory: async () => ({ + addStartupHook: () => {}, + addShutdownHook: () => {}, + }), }), ], }), diff --git a/packages/backend-defaults/src/CreateBackend.test.ts b/packages/backend-defaults/src/CreateBackend.test.ts index 8d56cd0986..946de90b2b 100644 --- a/packages/backend-defaults/src/CreateBackend.test.ts +++ b/packages/backend-defaults/src/CreateBackend.test.ts @@ -35,7 +35,10 @@ describe('createBackend', () => { createServiceFactory({ service: coreServices.rootLifecycle, deps: {}, - factory: async () => ({ addShutdownHook: () => {} }), + factory: async () => ({ + addStartupHook: () => {}, + addShutdownHook: () => {}, + }), }), ], }), @@ -49,12 +52,18 @@ describe('createBackend', () => { createServiceFactory({ service: coreServices.rootLifecycle, deps: {}, - factory: async () => ({ addShutdownHook: () => {} }), + factory: async () => ({ + addStartupHook: () => {}, + addShutdownHook: () => {}, + }), }), createServiceFactory({ service: coreServices.rootLifecycle, deps: {}, - factory: async () => ({ addShutdownHook: () => {} }), + factory: async () => ({ + addStartupHook: () => {}, + addShutdownHook: () => {}, + }), }), ], }), diff --git a/packages/backend-plugin-api/api-report.md b/packages/backend-plugin-api/api-report.md index 7dc6ae97b3..b3cbef993c 100644 --- a/packages/backend-plugin-api/api-report.md +++ b/packages/backend-plugin-api/api-report.md @@ -273,6 +273,10 @@ export interface LifecycleService { hook: LifecycleServiceShutdownHook, options?: LifecycleServiceShutdownOptions, ): void; + addStartupHook( + hook: LifecycleServiceStartupHook, + options?: LifecycleServiceStartupOptions, + ): void; } // @public (undocumented) @@ -283,6 +287,14 @@ export interface LifecycleServiceShutdownOptions { logger?: LoggerService; } +// @public (undocumented) +export type LifecycleServiceStartupHook = () => void | Promise; + +// @public (undocumented) +export interface LifecycleServiceStartupOptions { + logger?: LoggerService; +} + // @public export interface LoggerService { // (undocumented) diff --git a/packages/backend-plugin-api/src/services/definitions/LifecycleService.ts b/packages/backend-plugin-api/src/services/definitions/LifecycleService.ts index 89351194e6..e0641e0457 100644 --- a/packages/backend-plugin-api/src/services/definitions/LifecycleService.ts +++ b/packages/backend-plugin-api/src/services/definitions/LifecycleService.ts @@ -16,6 +16,21 @@ import { LoggerService } from './LoggerService'; +/** + * @public + */ +export type LifecycleServiceStartupHook = () => void | Promise; + +/** + * @public + */ +export interface LifecycleServiceStartupOptions { + /** + * Optional {@link LoggerService} that will be used for logging instead of the default logger. + */ + logger?: LoggerService; +} + /** * @public */ @@ -35,6 +50,20 @@ export interface LifecycleServiceShutdownOptions { * @public */ export interface LifecycleService { + /** + * Register a function to be called when the backend has been initialized. + * + * @remarks + * + * When used with plugin scope it will wait for the plugin itself to have been initialized. + * + * When used with root scope it will wait for all plugins to have been initialized. + */ + addStartupHook( + hook: LifecycleServiceStartupHook, + options?: LifecycleServiceStartupOptions, + ): void; + /** * Register a function to be called when the backend is shutting down. */ diff --git a/packages/backend-plugin-api/src/services/definitions/index.ts b/packages/backend-plugin-api/src/services/definitions/index.ts index 0122bc0be8..ab0f0b84b1 100644 --- a/packages/backend-plugin-api/src/services/definitions/index.ts +++ b/packages/backend-plugin-api/src/services/definitions/index.ts @@ -26,6 +26,8 @@ export type { DiscoveryService } from './DiscoveryService'; export type { HttpRouterService } from './HttpRouterService'; export type { LifecycleService, + LifecycleServiceStartupHook, + LifecycleServiceStartupOptions, LifecycleServiceShutdownHook, LifecycleServiceShutdownOptions, } from './LifecycleService'; From 2c9f67e6f1663c4b28e33350168a40269cc2ce67 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 24 May 2023 16:11:35 +0200 Subject: [PATCH 03/12] backend-app-api: introduce lifecycle middleware Signed-off-by: Patrik Oldsberg --- .changeset/selfish-olives-end.md | 5 + packages/backend-app-api/api-report.md | 13 +++ .../createLifecycleMiddleware.test.ts | 78 +++++++++++++++ .../httpRouter/createLifecycleMiddleware.ts | 98 +++++++++++++++++++ .../httpRouter/httpRouterServiceFactory.ts | 15 ++- .../implementations/httpRouter/index.ts | 2 + 6 files changed, 208 insertions(+), 3 deletions(-) create mode 100644 .changeset/selfish-olives-end.md create mode 100644 packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts create mode 100644 packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts diff --git a/.changeset/selfish-olives-end.md b/.changeset/selfish-olives-end.md new file mode 100644 index 0000000000..cb3778f316 --- /dev/null +++ b/.changeset/selfish-olives-end.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Introduced built-in middleware into the default `HttpService` implementation that throws a `ServiceNotAvailable` error when plugins aren't able to serve request. Also introduced a request stalling mechanism that pauses incoming request until plugins have been fully initialized. diff --git a/packages/backend-app-api/api-report.md b/packages/backend-app-api/api-report.md index 27b218ecb3..cde0ef64d2 100644 --- a/packages/backend-app-api/api-report.md +++ b/packages/backend-app-api/api-report.md @@ -78,6 +78,11 @@ export function createHttpServer( }, ): Promise; +// @public +export function createLifecycleMiddleware( + options: LifecycleMiddlewareOptions, +): RequestHandler; + // @public (undocumented) export function createSpecializedBackend( options: CreateSpecializedBackendOptions, @@ -170,6 +175,14 @@ export const identityServiceFactory: ( options?: IdentityFactoryOptions | undefined, ) => ServiceFactory; +// @public +export interface LifecycleMiddlewareOptions { + // (undocumented) + lifecycle: LifecycleService; + // (undocumented) + timeoutMs?: number; +} + // @public export const lifecycleServiceFactory: () => ServiceFactory< LifecycleService, diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts new file mode 100644 index 0000000000..e1091d247f --- /dev/null +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts @@ -0,0 +1,78 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { createLifecycleMiddleware } from './createLifecycleMiddleware'; +import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory'; +import { mockServices } from '@backstage/backend-test-utils'; +import { ServiceUnavailableError } from '@backstage/errors'; + +describe('createLifecycleMiddleware', () => { + it('should pause requests when plugin is not ready', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + + const middleware = createLifecycleMiddleware({ lifecycle }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + await lifecycle.startup(); + + // pending call + expect(next).toHaveBeenCalledWith(); + + // new call + const next2 = jest.fn(); + middleware({} as any, {} as any, next2); + expect(next2).toHaveBeenCalledWith(); + }); + + it('should throw ServiceUnavailableError after shutdown', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + const middleware = createLifecycleMiddleware({ lifecycle }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + await lifecycle.shutdown(); + + // pending call + expect(next).toHaveBeenCalledWith( + new ServiceUnavailableError('Service is shutting down'), + ); + + // new call + const next2 = jest.fn(); + middleware({} as any, {} as any, next2); + expect(next2).toHaveBeenCalledWith( + new ServiceUnavailableError('Service is shutting down'), + ); + }); + + it('should throw ServiceUnavailableError after timeout', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + const middleware = createLifecycleMiddleware({ lifecycle, timeoutMs: 1 }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + + await new Promise(r => setTimeout(r, 2)); + + expect(next).toHaveBeenCalledWith( + new ServiceUnavailableError('Service has not started up yet'), + ); + }); +}); diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts new file mode 100644 index 0000000000..b2a41bbb2f --- /dev/null +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts @@ -0,0 +1,98 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { LifecycleService } from '@backstage/backend-plugin-api'; +import { ServiceUnavailableError } from '@backstage/errors'; +import { RequestHandler } from 'express'; + +export const DEFAULT_TIMEOUT_MS = 5000; + +/** + * Options for {@link createLifecycleMiddleware}. + * @public + */ +export interface LifecycleMiddlewareOptions { + lifecycle: LifecycleService; + timeoutMs?: number; +} + +/** + * Creates a middleware that pauses requests until the service has started. + * + * @remarks + * + * Requests that arrive before the service has started will be paused until startup is complete. + * If the service does not start within the provided timeout, the request will be rejected with a + * {@link @backstage/errors#ServiceUnavailableError}. + * + * If the service is shutting down, all requests will be rejected with a + * {@link @backstage/errors#ServiceUnavailableError}. + * + * @public + */ +export function createLifecycleMiddleware( + options: LifecycleMiddlewareOptions, +): RequestHandler { + const { lifecycle, timeoutMs = DEFAULT_TIMEOUT_MS } = options; + + let state: 'init' | 'up' | 'down' = 'init'; + const waiting = new Set<{ + next: (err?: Error) => void; + timeout: NodeJS.Timeout; + }>(); + + lifecycle.addStartupHook(async () => { + if (state === 'init') { + state = 'up'; + for (const item of waiting) { + clearTimeout(item.timeout); + item.next(); + } + waiting.clear(); + } + }); + + lifecycle.addShutdownHook(async () => { + state = 'down'; + + for (const item of waiting) { + clearTimeout(item.timeout); + item.next(new ServiceUnavailableError('Service is shutting down')); + } + waiting.clear(); + }); + + return (_req, _res, next) => { + if (state === 'up') { + next(); + return; + } else if (state === 'down') { + next(new ServiceUnavailableError('Service is shutting down')); + return; + } + + const item = { + next, + timeout: setTimeout(() => { + if (waiting.delete(item)) { + next(new ServiceUnavailableError('Service has not started up yet')); + } + }, timeoutMs), + }; + + waiting.add(item); + }; +} diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts index bbd54b7ade..2ecdeffd77 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.ts @@ -15,10 +15,12 @@ */ import { - createServiceFactory, coreServices, + createServiceFactory, } from '@backstage/backend-plugin-api'; import { Handler } from 'express'; +import PromiseRouter from 'express-promise-router'; +import { createLifecycleMiddleware } from './createLifecycleMiddleware'; /** * @public @@ -36,14 +38,21 @@ export const httpRouterServiceFactory = createServiceFactory( service: coreServices.httpRouter, deps: { plugin: coreServices.pluginMetadata, + lifecycle: coreServices.lifecycle, rootHttpRouter: coreServices.rootHttpRouter, }, - async factory({ plugin, rootHttpRouter }) { + async factory({ plugin, rootHttpRouter, lifecycle }) { const getPath = options?.getPath ?? (id => `/api/${id}`); const path = getPath(plugin.getId()); + + const router = PromiseRouter(); + rootHttpRouter.use(path, router); + + router.use(createLifecycleMiddleware({ lifecycle })); + return { use(handler: Handler) { - rootHttpRouter.use(path, handler); + router.use(handler); }, }; }, diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/index.ts b/packages/backend-app-api/src/services/implementations/httpRouter/index.ts index 9dfb01c5be..928a232131 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/index.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/index.ts @@ -16,3 +16,5 @@ export { httpRouterServiceFactory } from './httpRouterServiceFactory'; export type { HttpRouterFactoryOptions } from './httpRouterServiceFactory'; +export { createLifecycleMiddleware } from './createLifecycleMiddleware'; +export type { LifecycleMiddlewareOptions } from './createLifecycleMiddleware'; From 0a7e305fe2c0a39ba827f7b0b46d719ed4b0c0ef Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:12:21 +0200 Subject: [PATCH 04/12] backend-app-api: backend init now checks for duplicates and inits modules without a plugin Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.ts | 70 ++++++++++++------- packages/backend-app-api/src/wiring/types.ts | 10 --- 2 files changed, 44 insertions(+), 36 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index a6d0e74866..f33254b025 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -22,20 +22,23 @@ import { } from '@backstage/backend-plugin-api'; import { BackendLifecycleImpl } from '../services/implementations/rootLifecycle/rootLifecycleServiceFactory'; import { BackendPluginLifecycleImpl } from '../services/implementations/lifecycle/lifecycleServiceFactory'; -import { - BackendRegisterInit, - EnumerableServiceHolder, - ServiceOrExtensionPoint, -} from './types'; +import { EnumerableServiceHolder, ServiceOrExtensionPoint } from './types'; // Direct internal import to avoid duplication // eslint-disable-next-line @backstage/no-forbidden-package-imports import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; +export interface BackendRegisterInit { + consumes: Set; + provides: Set; + init: { + deps: { [name: string]: ServiceOrExtensionPoint }; + func: (deps: { [name: string]: unknown }) => Promise; + }; +} + export class BackendInitializer { #startPromise?: Promise; #features = new Array(); - #pluginInits = new Array(); - #moduleInits = new Map>(); #extensionPoints = new Map, unknown>(); #serviceHolder: EnumerableServiceHolder; @@ -132,6 +135,9 @@ export class BackendInitializer { } } + const pluginInits = new Map(); + const moduleInits = new Map>(); + // Enumerate all features for (const feature of this.#features) { for (const r of feature.getRegistrations()) { @@ -150,20 +156,26 @@ export class BackendInitializer { } if (r.type === 'plugin') { - this.#pluginInits.push({ - id: r.pluginId, + if (pluginInits.has(r.pluginId)) { + throw new Error(`Plugin '${r.pluginId}' is already registered`); + } + pluginInits.set(r.pluginId, { provides, consumes: new Set(Object.values(r.init.deps)), init: r.init, }); } else { - let modules = this.#moduleInits.get(r.pluginId); + let modules = moduleInits.get(r.pluginId); if (!modules) { - modules = []; - this.#moduleInits.set(r.pluginId, modules); + modules = new Map(); + moduleInits.set(r.pluginId, modules); } - modules.push({ - id: `${r.pluginId}.${r.moduleId}`, + if (modules.has(r.moduleId)) { + throw new Error( + `Module '${r.moduleId}' for plugin '${r.pluginId}' is already registered`, + ); + } + modules.set(r.moduleId, { provides, consumes: new Set(Object.values(r.init.deps)), init: r.init, @@ -172,32 +184,38 @@ export class BackendInitializer { } } + const allPluginIds = [ + ...new Set([...pluginInits.keys(), ...moduleInits.keys()]), + ]; + // All plugins are initialized in parallel await Promise.all( - this.#pluginInits.map(async pluginInit => { + allPluginIds.map(async pluginId => { // Modules are initialized before plugins, so that they can provide extension to the plugin - const modules = this.#moduleInits.get(pluginInit.id) ?? []; + const modules = moduleInits.get(pluginId) ?? []; await Promise.all( - modules.map(async moduleInit => { + Array.from(modules.values()).map(async moduleInit => { const moduleDeps = await this.#getInitDeps( moduleInit.init.deps, - moduleInit.id, + pluginId, ); await moduleInit.init.func(moduleDeps); }), ); // Once all modules have been initialized, we can initialize the plugin itself - const pluginDeps = await this.#getInitDeps( - pluginInit.init.deps, - pluginInit.id, - ); - await pluginInit.init.func(pluginDeps); + const pluginInit = pluginInits.get(pluginId); + // We allow modules to be installed without the accompanying plugin, so the plugin may not exist + if (pluginInit) { + const pluginDeps = await this.#getInitDeps( + pluginInit.init.deps, + pluginId, + ); + await pluginInit.init.func(pluginDeps); + } // Once the plugin and all modules have been initialized, we can signal that the plugin has stared up successfully - const lifecycleService = await this.#getPluginLifecycleImpl( - pluginInit.id, - ); + const lifecycleService = await this.#getPluginLifecycleImpl(pluginId); await lifecycleService.startup(); }), ); diff --git a/packages/backend-app-api/src/wiring/types.ts b/packages/backend-app-api/src/wiring/types.ts index 2a91bf46b9..ffedb40324 100644 --- a/packages/backend-app-api/src/wiring/types.ts +++ b/packages/backend-app-api/src/wiring/types.ts @@ -30,16 +30,6 @@ export interface Backend { stop(): Promise; } -export interface BackendRegisterInit { - id: string; - consumes: Set; - provides: Set; - init: { - deps: { [name: string]: ServiceOrExtensionPoint }; - func: (deps: { [name: string]: unknown }) => Promise; - }; -} - /** * @public */ From be1379a4da05d5284e8b5ca3724bb64e9483a01a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:20:27 +0200 Subject: [PATCH 05/12] backend-app-api: fix for test not making root lifecycle service available Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index c00969a52c..133a404ce7 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -17,9 +17,11 @@ import { createServiceRef, createServiceFactory, + coreServices, } from '@backstage/backend-plugin-api'; import { BackendInitializer } from './BackendInitializer'; import { ServiceRegistry } from './ServiceRegistry'; +import { rootLifecycleServiceFactory } from '../services/implementations'; const rootRef = createServiceRef<{ x: number }>({ id: '1', @@ -30,6 +32,16 @@ const pluginRef = createServiceRef<{ x: number }>({ id: '2', }); +class MockLogger { + debug() {} + info() {} + warn() {} + error() {} + child() { + return this; + } +} + describe('BackendInitializer', () => { it('should initialize root scoped services', async () => { const rootFactory = jest.fn(); @@ -46,6 +58,12 @@ describe('BackendInitializer', () => { deps: {}, factory: pluginFactory, })(), + rootLifecycleServiceFactory(), + createServiceFactory({ + service: coreServices.rootLogger, + deps: {}, + factory: () => new MockLogger(), + })(), ]); const init = new BackendInitializer(registry); From dfa9a0ea04f5dced8bece82d16b990f825a82f9c Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:27:34 +0200 Subject: [PATCH 06/12] backend-app-api: switch lifecycle middleware timeout to startupRequestPauseTimeout Signed-off-by: Patrik Oldsberg --- packages/backend-app-api/api-report.md | 4 +-- .../createLifecycleMiddleware.test.ts | 5 ++- .../httpRouter/createLifecycleMiddleware.ts | 33 +++++++++++++++++-- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/packages/backend-app-api/api-report.md b/packages/backend-app-api/api-report.md index cde0ef64d2..5f452f9e03 100644 --- a/packages/backend-app-api/api-report.md +++ b/packages/backend-app-api/api-report.md @@ -18,6 +18,7 @@ import { Handler } from 'express'; import { HelmetOptions } from 'helmet'; import * as http from 'http'; import { HttpRouterService } from '@backstage/backend-plugin-api'; +import { HumanDuration } from '@backstage/types'; import { IdentityService } from '@backstage/backend-plugin-api'; import { JsonObject } from '@backstage/types'; import { LifecycleService } from '@backstage/backend-plugin-api'; @@ -179,8 +180,7 @@ export const identityServiceFactory: ( export interface LifecycleMiddlewareOptions { // (undocumented) lifecycle: LifecycleService; - // (undocumented) - timeoutMs?: number; + startupRequestPauseTimeout?: HumanDuration; } // @public diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts index e1091d247f..31eecf14bb 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.test.ts @@ -63,7 +63,10 @@ describe('createLifecycleMiddleware', () => { it('should throw ServiceUnavailableError after timeout', async () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - const middleware = createLifecycleMiddleware({ lifecycle, timeoutMs: 1 }); + const middleware = createLifecycleMiddleware({ + lifecycle, + startupRequestPauseTimeout: { milliseconds: 1 }, + }); const next = jest.fn(); middleware({} as any, {} as any, next); diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts index b2a41bbb2f..e5e829eebe 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts @@ -16,9 +16,31 @@ import { LifecycleService } from '@backstage/backend-plugin-api'; import { ServiceUnavailableError } from '@backstage/errors'; +import { HumanDuration } from '@backstage/types'; import { RequestHandler } from 'express'; -export const DEFAULT_TIMEOUT_MS = 5000; +export const DEFAULT_TIMEOUT = { seconds: 5 }; + +function durationToMs(duration: HumanDuration): number { + const { + years = 0, + months = 0, + weeks = 0, + days = 0, + hours = 0, + minutes = 0, + seconds = 0, + milliseconds = 0, + } = duration; + + const totalDays = years * 365 + months * 30 + weeks * 7 + days; + const totalHours = totalDays * 24 + hours; + const totalMinutes = totalHours * 60 + minutes; + const totalSeconds = totalMinutes * 60 + seconds; + const totalMilliseconds = totalSeconds * 1000 + milliseconds; + + return totalMilliseconds; +} /** * Options for {@link createLifecycleMiddleware}. @@ -26,7 +48,10 @@ export const DEFAULT_TIMEOUT_MS = 5000; */ export interface LifecycleMiddlewareOptions { lifecycle: LifecycleService; - timeoutMs?: number; + /** + * The maximum time that paused requests will wait for the service to start, before returning an error. + */ + startupRequestPauseTimeout?: HumanDuration; } /** @@ -46,7 +71,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, timeoutMs = DEFAULT_TIMEOUT_MS } = options; + const { lifecycle, startupRequestPauseTimeout = DEFAULT_TIMEOUT } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -75,6 +100,8 @@ export function createLifecycleMiddleware( waiting.clear(); }); + const timeoutMs = durationToMs(startupRequestPauseTimeout); + return (_req, _res, next) => { if (state === 'up') { next(); From 0a213ab691111b85fa2015bb269d1313fc3d9521 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:30:10 +0200 Subject: [PATCH 07/12] backend-app-api: log root lifecycle hook success with debug level Signed-off-by: Patrik Oldsberg --- .../rootLifecycle/rootLifecycleServiceFactory.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts index 045c5e8267..39e66458c3 100644 --- a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts @@ -48,13 +48,13 @@ export class BackendLifecycleImpl implements RootLifecycleService { } this.#hasStarted = true; - this.logger.info(`Running ${this.#startupTasks.length} startup tasks...`); + this.logger.debug(`Running ${this.#startupTasks.length} startup tasks...`); await Promise.all( this.#startupTasks.map(async ({ hook, options }) => { const logger = options?.logger ?? this.logger; try { await hook(); - logger.info(`Startup hook succeeded`); + logger.debug(`Startup hook succeeded`); } catch (error) { logger.error(`Startup hook failed, ${error}`); } @@ -81,13 +81,15 @@ export class BackendLifecycleImpl implements RootLifecycleService { } this.#hasShutdown = true; - this.logger.info(`Running ${this.#shutdownTasks.length} shutdown tasks...`); + this.logger.debug( + `Running ${this.#shutdownTasks.length} shutdown tasks...`, + ); await Promise.all( this.#shutdownTasks.map(async ({ hook, options }) => { const logger = options?.logger ?? this.logger; try { await hook(); - logger.info(`Shutdown hook succeeded`); + logger.debug(`Shutdown hook succeeded`); } catch (error) { logger.error(`Shutdown hook failed, ${error}`); } From 0f02bdcbd72d09411bbf0ca1dfbf536a35cd2e24 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:38:25 +0200 Subject: [PATCH 08/12] backend-app-api: more informative error messages for plugin and module startup failures Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 43 +++++++++++++++++++ .../src/wiring/BackendInitializer.ts | 17 ++++++-- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index 133a404ce7..a8b302b905 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -18,6 +18,8 @@ import { createServiceRef, createServiceFactory, coreServices, + createBackendPlugin, + createBackendModule, } from '@backstage/backend-plugin-api'; import { BackendInitializer } from './BackendInitializer'; import { ServiceRegistry } from './ServiceRegistry'; @@ -72,4 +74,45 @@ describe('BackendInitializer', () => { expect(rootFactory).toHaveBeenCalled(); expect(pluginFactory).not.toHaveBeenCalled(); }); + + it('should forward errors when plugins fail to start', async () => { + const init = new BackendInitializer(new ServiceRegistry([])); + init.add( + createBackendPlugin({ + pluginId: 'test', + register(reg) { + reg.registerInit({ + deps: {}, + async init() { + throw new Error('NOPE'); + }, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Plugin 'test' startup failed; caused by Error: NOPE", + ); + }); + + it('should forward errors when modules fail to start', async () => { + const init = new BackendInitializer(new ServiceRegistry([])); + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'mod', + register(reg) { + reg.registerInit({ + deps: {}, + async init() { + throw new Error('NOPE'); + }, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Module 'mod' for plugin 'test' startup failed; caused by Error: NOPE", + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index f33254b025..8c2b75e887 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -26,6 +26,7 @@ import { EnumerableServiceHolder, ServiceOrExtensionPoint } from './types'; // Direct internal import to avoid duplication // eslint-disable-next-line @backstage/no-forbidden-package-imports import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; +import { ForwardedError } from '@backstage/errors'; export interface BackendRegisterInit { consumes: Set; @@ -194,12 +195,17 @@ export class BackendInitializer { // Modules are initialized before plugins, so that they can provide extension to the plugin const modules = moduleInits.get(pluginId) ?? []; await Promise.all( - Array.from(modules.values()).map(async moduleInit => { + Array.from(modules).map(async ([moduleId, moduleInit]) => { const moduleDeps = await this.#getInitDeps( moduleInit.init.deps, pluginId, ); - await moduleInit.init.func(moduleDeps); + await moduleInit.init.func(moduleDeps).catch(error => { + throw new ForwardedError( + `Module '${moduleId}' for plugin '${pluginId}' startup failed`, + error, + ); + }); }), ); @@ -211,7 +217,12 @@ export class BackendInitializer { pluginInit.init.deps, pluginId, ); - await pluginInit.init.func(pluginDeps); + await pluginInit.init.func(pluginDeps).catch(error => { + throw new ForwardedError( + `Plugin '${pluginId}' startup failed`, + error, + ); + }); } // Once the plugin and all modules have been initialized, we can signal that the plugin has stared up successfully From aa1c031b7b5cde240bdc60ab980d08a2a20af6ff Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 14:51:49 +0200 Subject: [PATCH 09/12] backend-app-api: added tests for plugin and module duplication Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index a8b302b905..b05194f594 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -115,4 +115,64 @@ describe('BackendInitializer', () => { "Module 'mod' for plugin 'test' startup failed; caused by Error: NOPE", ); }); + + it('should reject duplicate plugins', async () => { + const init = new BackendInitializer(new ServiceRegistry([])); + init.add( + createBackendPlugin({ + pluginId: 'test', + register(reg) { + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + })(), + ); + init.add( + createBackendPlugin({ + pluginId: 'test', + register(reg) { + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Plugin 'test' is already registered", + ); + }); + + it('should reject duplicate modules', async () => { + const init = new BackendInitializer(new ServiceRegistry([])); + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'mod', + register(reg) { + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + })(), + ); + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'mod', + register(reg) { + reg.registerInit({ + deps: {}, + async init() {}, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Module 'mod' for plugin 'test' is already registered", + ); + }); }); From f26a927f48589568f9d3e76b9011545ee80c38ab Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 17:30:35 +0200 Subject: [PATCH 10/12] backend-app-api: fix httpRouterServiceFactory tests Signed-off-by: Patrik Oldsberg --- .../httpRouterServiceFactory.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.test.ts b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.test.ts index e52b4c9a36..d0b40df33b 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.test.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/httpRouterServiceFactory.test.ts @@ -26,24 +26,32 @@ describe('httpRouterFactory', () => { { rootHttpRouter, plugin: { getId: () => 'test1' }, + lifecycle: { addStartupHook() {}, addShutdownHook() {} }, }, undefined, ); router1.use(handler1); expect(rootHttpRouter.use).toHaveBeenCalledTimes(1); - expect(rootHttpRouter.use).toHaveBeenCalledWith('/api/test1', handler1); + expect(rootHttpRouter.use).toHaveBeenCalledWith( + '/api/test1', + expect.any(Function), + ); const handler2 = () => {}; const router2 = await factory.factory( { rootHttpRouter, plugin: { getId: () => 'test2' }, + lifecycle: { addStartupHook() {}, addShutdownHook() {} }, }, undefined, ); router2.use(handler2); expect(rootHttpRouter.use).toHaveBeenCalledTimes(2); - expect(rootHttpRouter.use).toHaveBeenCalledWith('/api/test2', handler2); + expect(rootHttpRouter.use).toHaveBeenCalledWith( + '/api/test2', + expect.any(Function), + ); }); it('should use custom path generator', async () => { @@ -57,6 +65,7 @@ describe('httpRouterFactory', () => { { rootHttpRouter, plugin: { getId: () => 'test1' }, + lifecycle: { addStartupHook() {}, addShutdownHook() {} }, }, undefined, ); @@ -64,7 +73,7 @@ describe('httpRouterFactory', () => { expect(rootHttpRouter.use).toHaveBeenCalledTimes(1); expect(rootHttpRouter.use).toHaveBeenCalledWith( '/some/test1/path', - handler1, + expect.any(Function), ); const handler2 = () => {}; @@ -72,6 +81,7 @@ describe('httpRouterFactory', () => { { rootHttpRouter, plugin: { getId: () => 'test2' }, + lifecycle: { addStartupHook() {}, addShutdownHook() {} }, }, undefined, ); @@ -79,7 +89,7 @@ describe('httpRouterFactory', () => { expect(rootHttpRouter.use).toHaveBeenCalledTimes(2); expect(rootHttpRouter.use).toHaveBeenCalledWith( '/some/test2/path', - handler2, + expect.any(Function), ); }); }); From 5f28097f5d59c3341b7be63fe95bb43a3c04b4d7 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 25 May 2023 17:43:48 +0200 Subject: [PATCH 11/12] backend-app-api: attempting to add a lifecycle hook after trigger now throws Signed-off-by: Patrik Oldsberg --- .../lifecycle/lifecycleServiceFactory.ts | 3 +++ .../rootLifecycleServiceFactory.test.ts | 13 +++++++++++++ .../rootLifecycle/rootLifecycleServiceFactory.ts | 6 ++++++ 3 files changed, 22 insertions(+) diff --git a/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts b/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts index c0cfcf7d8c..2b68a81807 100644 --- a/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/lifecycle/lifecycleServiceFactory.ts @@ -44,6 +44,9 @@ export class BackendPluginLifecycleImpl implements LifecycleService { hook: LifecycleServiceStartupHook, options?: LifecycleServiceStartupOptions, ): void { + if (this.#hasStarted) { + throw new Error('Attempted to add startup hook after startup'); + } this.#startupTasks.push({ hook, options }); } diff --git a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.test.ts b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.test.ts index 8d2b2af3d5..91cb9031ae 100644 --- a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.test.ts +++ b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.test.ts @@ -44,4 +44,17 @@ describe('lifecycleService', () => { }); await expect(service.shutdown()).resolves.toBeUndefined(); }); + + it('should reject hooks after trigger', async () => { + const service = new BackendLifecycleImpl(getVoidLogger()); + await service.startup(); + expect(() => { + service.addStartupHook(() => {}); + }).toThrow('Attempted to add startup hook after startup'); + + await service.shutdown(); + expect(() => { + service.addShutdownHook(() => {}); + }).toThrow('Attempted to add shutdown hook after shutdown'); + }); }); diff --git a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts index 39e66458c3..197f99b97a 100644 --- a/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-app-api/src/services/implementations/rootLifecycle/rootLifecycleServiceFactory.ts @@ -39,6 +39,9 @@ export class BackendLifecycleImpl implements RootLifecycleService { hook: LifecycleServiceStartupHook, options?: LifecycleServiceStartupOptions, ): void { + if (this.#hasStarted) { + throw new Error('Attempted to add startup hook after startup'); + } this.#startupTasks.push({ hook, options }); } @@ -72,6 +75,9 @@ export class BackendLifecycleImpl implements RootLifecycleService { hook: LifecycleServiceShutdownHook, options?: LifecycleServiceShutdownOptions, ): void { + if (this.#hasShutdown) { + throw new Error('Attempted to add shutdown hook after shutdown'); + } this.#shutdownTasks.push({ hook, options }); } From 0941102161393323e4aa29b0fd05eff34ae3b503 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 29 May 2023 12:04:09 +0200 Subject: [PATCH 12/12] backend-app-api: document default lifecycle middleware timeout Signed-off-by: Patrik Oldsberg --- .../implementations/httpRouter/createLifecycleMiddleware.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts index e5e829eebe..958ae33e99 100644 --- a/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-app-api/src/services/implementations/httpRouter/createLifecycleMiddleware.ts @@ -50,6 +50,8 @@ export interface LifecycleMiddlewareOptions { lifecycle: LifecycleService; /** * The maximum time that paused requests will wait for the service to start, before returning an error. + * + * Defaults to 5 seconds. */ startupRequestPauseTimeout?: HumanDuration; }