From 29180ec3d4ac400a7a594278823cfb8c73d2898a Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 11 Dec 2024 12:16:43 +0100 Subject: [PATCH] fix: move startup lifecycle back to plugin service Signed-off-by: Camila Belo --- .changeset/curvy-fishes-rule.md | 5 +++ .changeset/fast-tools-fix.md | 10 +++++ .changeset/rude-pears-tie.md | 1 - .../backend-defaults/report-httpRouter.api.md | 4 +- .../report-rootHttpRouter.api.md | 2 - .../createLifecycleMiddleware.test.ts | 27 ------------- .../createLifecycleMiddleware.ts | 18 +-------- .../http/createLifecycleMiddleware.test.ts | 18 +++++++-- .../http/createLifecycleMiddleware.ts | 26 +++++++++---- .../httpRouter/httpRouterServiceFactory.ts | 5 ++- .../rootHttpRouterServiceFactory.test.ts | 24 +++++------- .../rootHttpRouterServiceFactory.ts | 38 +++++++------------ 12 files changed, 77 insertions(+), 101 deletions(-) create mode 100644 .changeset/curvy-fishes-rule.md create mode 100644 .changeset/fast-tools-fix.md rename packages/backend-defaults/src/entrypoints/{rootHttpRouter => httpRouter}/createLifecycleMiddleware.test.ts (71%) rename packages/backend-defaults/src/entrypoints/{rootHttpRouter => httpRouter}/createLifecycleMiddleware.ts (85%) diff --git a/.changeset/curvy-fishes-rule.md b/.changeset/curvy-fishes-rule.md new file mode 100644 index 0000000000..2ce535166d --- /dev/null +++ b/.changeset/curvy-fishes-rule.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-defaults': patch +--- + +Fix server response time by moving the lifecycle startup hooks back to the plugin lifecycle service. diff --git a/.changeset/fast-tools-fix.md b/.changeset/fast-tools-fix.md new file mode 100644 index 0000000000..25dd292c82 --- /dev/null +++ b/.changeset/fast-tools-fix.md @@ -0,0 +1,10 @@ +--- +'@backstage/backend-defaults': minor +--- + +**BREAKING CHANGE**: The `LifecycleMiddlewareOptions.startupRequestPauseTimeout` has been removed. Use the `backend.lifecycle.startupRequestPauseTimeout` setting in your `app-config.yaml` file to customize how the `createLifecycleMiddleware` function should behave. Also the root config service is required as an options when calling the `createLifecycleMiddleware` function: + +```diff +- createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout }) ++ createLifecycleMiddleware({ config, lifecycle }) +``` diff --git a/.changeset/rude-pears-tie.md b/.changeset/rude-pears-tie.md index c0cb970727..e29d517959 100644 --- a/.changeset/rude-pears-tie.md +++ b/.changeset/rude-pears-tie.md @@ -3,4 +3,3 @@ --- Remove use of the `stoppable` library on the `DefaultRootHttpRouterService` as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests. -Also, we pass a new `lifecycleMiddleware` to the `rootHttpRouterServiceFactory` configure function that must be called manually if you don't call `applyDefaults`. diff --git a/packages/backend-defaults/report-httpRouter.api.md b/packages/backend-defaults/report-httpRouter.api.md index 4f6c6f06dc..55c3f0b208 100644 --- a/packages/backend-defaults/report-httpRouter.api.md +++ b/packages/backend-defaults/report-httpRouter.api.md @@ -10,7 +10,6 @@ import express from 'express'; import { HttpAuthService } from '@backstage/backend-plugin-api'; import { HttpRouterService } from '@backstage/backend-plugin-api'; import { HttpRouterServiceAuthPolicy } from '@backstage/backend-plugin-api'; -import { HumanDuration } from '@backstage/types'; import { LifecycleService } from '@backstage/backend-plugin-api'; import { RequestHandler } from 'express'; import { RootConfigService } from '@backstage/backend-plugin-api'; @@ -51,9 +50,10 @@ export const httpRouterServiceFactory: ServiceFactory< // @public export interface LifecycleMiddlewareOptions { + // (undocumented) + config: RootConfigService; // (undocumented) lifecycle: LifecycleService; - startupRequestPauseTimeout?: HumanDuration; } // (No @packageDocumentation comment for this package) diff --git a/packages/backend-defaults/report-rootHttpRouter.api.md b/packages/backend-defaults/report-rootHttpRouter.api.md index 4489fed107..41830ebf17 100644 --- a/packages/backend-defaults/report-rootHttpRouter.api.md +++ b/packages/backend-defaults/report-rootHttpRouter.api.md @@ -134,8 +134,6 @@ export interface RootHttpRouterConfigureContext { // (undocumented) lifecycle: LifecycleService; // (undocumented) - lifecycleMiddleware: RequestHandler; - // (undocumented) logger: LoggerService; // (undocumented) middleware: MiddlewareFactory; diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts similarity index 71% rename from packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts rename to packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts index 76af28cc6e..31eecf14bb 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.test.ts @@ -78,31 +78,4 @@ describe('createLifecycleMiddleware', () => { new ServiceUnavailableError('Service has not started up yet'), ); }); - - it('should delay service shutdown for the default timeout duration', async () => { - jest.useFakeTimers(); - const defaultTimeout = 30000; - const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - createLifecycleMiddleware({ lifecycle }); - const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { - jest.useRealTimers(); - }); - jest.advanceTimersByTime(defaultTimeout); - return expect(beforeShutdownPromise).resolves.toBeUndefined(); - }); - - it('should delay service shutdown for the configured timeout duration - time in human duration', async () => { - jest.useFakeTimers(); - const configuredTimeout = 20000; - const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - createLifecycleMiddleware({ - lifecycle, - serverShutdownDelay: { milliseconds: configuredTimeout }, - }); - const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { - jest.useRealTimers(); - }); - jest.advanceTimersByTime(configuredTimeout); - return expect(beforeShutdownPromise).resolves.toBeUndefined(); - }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts similarity index 85% rename from packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts rename to packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts index 3745f7f48d..99fb82a41e 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/createLifecycleMiddleware.ts @@ -34,12 +34,6 @@ export interface LifecycleMiddlewareOptions { * Defaults to 5 seconds. */ startupRequestPauseTimeout?: HumanDuration; - /** - * The maximum time that the server will wait for stop accepting traffic, before returning an error. - * - * Defaults to 0 seconds. - */ - serverShutdownDelay?: HumanDuration; } /** @@ -59,8 +53,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout, serverShutdownDelay } = - options; + const { lifecycle, startupRequestPauseTimeout } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -79,15 +72,6 @@ export function createLifecycleMiddleware( } }); - lifecycle.addBeforeShutdownHook(async () => { - const timeoutMs = durationToMilliseconds( - serverShutdownDelay ?? DEFAULT_SERVER_SHUTDOWN_TIMEOUT, - ); - return await new Promise(resolve => { - setTimeout(resolve, timeoutMs); - }); - }); - lifecycle.addShutdownHook(async () => { state = 'down'; for (const item of waiting) { diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts index 3b85c8cb92..36ca2da61b 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.test.ts @@ -20,10 +20,14 @@ import { mockServices } from '@backstage/backend-test-utils'; import { ServiceUnavailableError } from '@backstage/errors'; describe('createLifecycleMiddleware', () => { + const config = mockServices.rootConfig.mock(); it('should pause requests when plugin is not ready', async () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - const middleware = createLifecycleMiddleware({ lifecycle }); + const middleware = createLifecycleMiddleware({ + config, + lifecycle, + }); const next = jest.fn(); middleware({} as any, {} as any, next); @@ -41,7 +45,7 @@ describe('createLifecycleMiddleware', () => { it('should throw ServiceUnavailableError after shutdown', async () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); - const middleware = createLifecycleMiddleware({ lifecycle }); + const middleware = createLifecycleMiddleware({ config, lifecycle }); const next = jest.fn(); middleware({} as any, {} as any, next); @@ -65,7 +69,15 @@ describe('createLifecycleMiddleware', () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); const middleware = createLifecycleMiddleware({ lifecycle, - startupRequestPauseTimeout: { milliseconds: 1 }, + config: mockServices.rootConfig({ + data: { + backend: { + lifecycle: { + startupRequestPauseTimeout: { milliseconds: 1 }, + }, + }, + }, + }), }); const next = jest.fn(); diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts index 3f8ee3ea69..444b0c08f9 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/http/createLifecycleMiddleware.ts @@ -14,7 +14,11 @@ * limitations under the License. */ -import { LifecycleService } from '@backstage/backend-plugin-api'; +import { + RootConfigService, + LifecycleService, +} from '@backstage/backend-plugin-api'; +import { readDurationFromConfig } from '@backstage/config'; import { ServiceUnavailableError } from '@backstage/errors'; import { HumanDuration, durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; @@ -26,13 +30,8 @@ export const DEFAULT_TIMEOUT = { seconds: 5 }; * @public */ export interface LifecycleMiddlewareOptions { + config: RootConfigService; 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; } /** @@ -52,7 +51,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout = DEFAULT_TIMEOUT } = options; + const { lifecycle } = options; let state: 'init' | 'up' | 'down' = 'init'; const waiting = new Set<{ @@ -81,6 +80,17 @@ export function createLifecycleMiddleware( waiting.clear(); }); + let startupRequestPauseTimeout: HumanDuration = DEFAULT_TIMEOUT; + + if ( + 'config' in options && + options.config.has('backend.lifecycle.startupRequestPauseTimeout') + ) { + startupRequestPauseTimeout = readDurationFromConfig(options.config, { + key: 'backend.lifecycle.startupRequestPauseTimeout', + }); + } + const timeoutMs = durationToMilliseconds(startupRequestPauseTimeout); return (_req, _res, next) => { diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts index a448827e16..6f8792aa3d 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts @@ -22,6 +22,7 @@ import { HttpRouterServiceAuthPolicy, } from '@backstage/backend-plugin-api'; import { + createLifecycleMiddleware, createCookieAuthRefreshMiddleware, createCredentialsBarrier, createAuthIntegrationRouter, @@ -42,11 +43,12 @@ export const httpRouterServiceFactory = createServiceFactory({ deps: { plugin: coreServices.pluginMetadata, config: coreServices.rootConfig, + lifecycle: coreServices.lifecycle, rootHttpRouter: coreServices.rootHttpRouter, auth: coreServices.auth, httpAuth: coreServices.httpAuth, }, - async factory({ auth, httpAuth, config, plugin, rootHttpRouter }) { + async factory({ auth, httpAuth, config, plugin, rootHttpRouter, lifecycle }) { const router = PromiseRouter(); rootHttpRouter.use(`/api/${plugin.getId()}`, router); @@ -57,6 +59,7 @@ export const httpRouterServiceFactory = createServiceFactory({ }); router.use(createAuthIntegrationRouter({ auth })); + router.use(createLifecycleMiddleware({ config, lifecycle })); router.use(credentialsBarrier.middleware); router.use(createCookieAuthRefreshMiddleware({ auth, httpAuth })); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index fb0d6cdad0..0f2b6463e4 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -207,20 +207,21 @@ describe('rootHttpRouterServiceFactory', () => { it('should wait the server to shutdown', async () => { jest.useFakeTimers(); + const serverStopMock = jest.fn(); + let app: Express | undefined = undefined; const lifecycleMock = new BackendLifecycleImpl(mockServices.rootLogger()); const tester = ServiceFactoryTester.from( rootHttpRouterServiceFactory({ configure(options) { - console.log('configure'); options.app.use(options.healthRouter); - options.app.use(options.lifecycleMiddleware); options.app.get('/test', (_req, res) => { res.status(200).send({ status: 'ok' }).end(); }); options.app.use(options.middleware.error()); app = options.app; + options.server.addListener('close', serverStopMock); }, }), { @@ -306,18 +307,6 @@ describe('rootHttpRouterServiceFactory', () => { jest.advanceTimersByTime(1); - // No longer accepting requests after shutdown - await request(app!) - .get('/test') - .expect(503, { - error: { - name: 'ServiceUnavailableError', - message: 'Service is shutting down', - }, - request: { method: 'GET', url: '/test' }, - response: { statusCode: 503 }, - }); - await request(app) .get('/.backstage/health/v1/liveness') .expect(200, { status: 'ok' }); @@ -327,6 +316,11 @@ describe('rootHttpRouterServiceFactory', () => { status: 'error', }); - return await expect(beforeShutdownPromise).resolves.toBeUndefined(); + return expect( + beforeShutdownPromise.then(() => { + expect(serverStopMock).toHaveBeenCalled(); + jest.useRealTimers(); + }), + ).resolves.toBeUndefined(); }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 4afdfb1ec2..e5c64124fd 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -30,9 +30,8 @@ import { } from './http'; import { DefaultRootHttpRouter } from './DefaultRootHttpRouter'; import { createHealthRouter } from './createHealthRouter'; -import { createLifecycleMiddleware } from './createLifecycleMiddleware'; +import { durationToMilliseconds } from '@backstage/types'; import { readDurationFromConfig } from '@backstage/config'; -import { HumanDuration } from '@backstage/types'; /** * @public @@ -46,7 +45,6 @@ export interface RootHttpRouterConfigureContext { logger: LoggerService; lifecycle: LifecycleService; healthRouter: RequestHandler; - lifecycleMiddleware: RequestHandler; applyDefaults: () => void; } @@ -95,26 +93,6 @@ const rootHttpRouterServiceFactoryWithOptions = ( const healthRouter = createHealthRouter({ config, health }); - let startupRequestPauseTimeout: HumanDuration | undefined; - if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { - startupRequestPauseTimeout = readDurationFromConfig(config, { - key: 'backend.lifecycle.startupRequestPauseTimeout', - }); - } - - let serverShutdownDelay: HumanDuration | undefined; - if (config.has('backend.lifecycle.serverShutdownDelay')) { - serverShutdownDelay = readDurationFromConfig(config, { - key: 'backend.lifecycle.serverShutdownDelay', - }); - } - - const lifecycleMiddleware = createLifecycleMiddleware({ - lifecycle, - startupRequestPauseTimeout, - serverShutdownDelay, - }); - const server = await createHttpServer( app, readHttpServerOptions(config.getOptionalConfig('backend')), @@ -130,7 +108,6 @@ const rootHttpRouterServiceFactoryWithOptions = ( logger, lifecycle, healthRouter, - lifecycleMiddleware, applyDefaults() { if (process.env.NODE_ENV === 'development') { app.set('json spaces', 2); @@ -140,13 +117,24 @@ const rootHttpRouterServiceFactoryWithOptions = ( app.use(middleware.compression()); app.use(middleware.logging()); app.use(healthRouter); - app.use(lifecycleMiddleware); app.use(routes); app.use(middleware.notFound()); app.use(middleware.error()); }, }); + if (config.has('backend.lifecycle.serverShutdownDelay')) { + const serverShutdownDelay = readDurationFromConfig(config, { + key: 'backend.lifecycle.serverShutdownDelay', + }); + lifecycle.addBeforeShutdownHook(async () => { + const timeoutMs = durationToMilliseconds(serverShutdownDelay); + return await new Promise(resolve => { + setTimeout(resolve, timeoutMs); + }); + }); + } + lifecycle.addShutdownHook(() => server.stop()); await server.start();