paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor: address latest review comments

Browse Source 
Signed-off-by: Camila Belo <camilaibs@gmail.com>
This commit is contained in:
Camila Belo
2024-12-09 18:43:38 +01:00
parent ad39962dc9
commit d0cbd82965
6 changed files with 144 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.changeset/rude-pears-tie.md
+5
View File
@@ -0,0 +1,5 @@
---
'@backstage/backend-defaults': patch
---
Remove use of the `stoppable` library as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests.
packages/backend-defaults/config.d.ts
Vendored
+4 -2
View File
@@ -31,14 +31,16 @@ export interface Config {
lifecycle?: {
/**
* The maximum time that paused requests will wait for the service to start, before returning an error.
* If you pass a number or string, it will be interpreted as milliseconds.
* Defaults to 5 seconds.
*/
startupRequestPauseTimeout?: HumanDuration;
startupRequestPauseTimeout?: number | string | HumanDuration;
/**
* The maximum time that the server will wait for stop accepting traffic, before returning an error.
* If you pass a number or string, it will be interpreted as milliseconds.
* Defaults to 30 seconds.
*/
shutdownRequestPauseTimeout?: HumanDuration;
shutdownRequestDelayTimeout?: number | string | HumanDuration;
};
/** Address that the backend should listen to. */
packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts
+1 -1
View File
@@ -97,7 +97,7 @@ describe('createLifecycleMiddleware', () => {
const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger());
createLifecycleMiddleware({
lifecycle,
shutdownRequestPauseTimeout: { milliseconds: configuredTimeout },
shutdownRequestDelayTimeout: { milliseconds: configuredTimeout },
});
const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => {
jest.useRealTimers();
packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts
+4 -4
View File
@@ -20,7 +20,7 @@ import { HumanDuration, durationToMilliseconds } from '@backstage/types';
import { RequestHandler } from 'express';
export const DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT = { seconds: 5 };
export const DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT = { seconds: 30 };
export const DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT = { seconds: 30 };
/**
* Options for {@link createLifecycleMiddleware}.
@@ -39,7 +39,7 @@ export interface LifecycleMiddlewareOptions {
*
* Defaults to 30 seconds.
*/
shutdownRequestPauseTimeout?: HumanDuration;
shutdownRequestDelayTimeout?: HumanDuration;
}
/**
@@ -59,7 +59,7 @@ export interface LifecycleMiddlewareOptions {
export function createLifecycleMiddleware(
options: LifecycleMiddlewareOptions,
): RequestHandler {
const { lifecycle, startupRequestPauseTimeout, shutdownRequestPauseTimeout } =
const { lifecycle, startupRequestPauseTimeout, shutdownRequestDelayTimeout } =
options;
let state: 'init' | 'up' | 'down' = 'init';
@@ -81,7 +81,7 @@ export function createLifecycleMiddleware(
lifecycle.addBeforeShutdownHook(async () => {
const timeoutMs = durationToMilliseconds(
shutdownRequestPauseTimeout ?? DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT,
shutdownRequestDelayTimeout ?? DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT,
);
return await new Promise(resolve => {
setTimeout(resolve, timeoutMs);
packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts
+102 -4
View File
@@ -20,13 +20,55 @@ import {
} from '@backstage/backend-test-utils';
import { Express } from 'express';
import request from 'supertest';
import { rootHttpRouterServiceFactory } from './rootHttpRouterServiceFactory';
import {
getConfigInHumanDuration,
rootHttpRouterServiceFactory,
} from './rootHttpRouterServiceFactory';
import {
ServiceFactory,
coreServices,
createServiceFactory,
} from '@backstage/backend-plugin-api';
import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory';
import { ConfigReader } from '@backstage/config';
describe('getConfigInHumanDuration', () => {
const values = ['20000', 20000, { milliseconds: 20000 }];
it.each(values)(
'should parse the lifecycle startup request timeout from a %s config value',
async value => {
const key = 'backend.lifecycle.startupRequestPauseTimeout';
const config = new ConfigReader({
backend: {
lifecycle: {
startupRequestPauseTimeout: value,
},
},
});
expect(getConfigInHumanDuration(config, key)).toMatchObject({
milliseconds: 20000,
});
},
);
it.each(values)(
'should parse the lifecycle shutdown delay timeout from a %s config value',
async value => {
const key = 'backend.lifecycle.shutdownRequestDelayTimeout';
const config = new ConfigReader({
backend: {
lifecycle: {
shutdownRequestDelayTimeout: value,
},
},
});
expect(getConfigInHumanDuration(config, key)).toMatchObject({
milliseconds: 20000,
});
},
);
});
async function createExpressApp(...dependencies: ServiceFactory[]) {
let app: Express | undefined = undefined;
@@ -204,7 +246,7 @@ describe('rootHttpRouterServiceFactory', () => {
);
});
it('should wait the server shutdown', async () => {
it('should wait the server to shutdown', async () => {
jest.useFakeTimers();
let app: Express | undefined = undefined;
@@ -214,10 +256,12 @@ describe('rootHttpRouterServiceFactory', () => {
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;
},
}),
@@ -254,19 +298,73 @@ describe('rootHttpRouterServiceFactory', () => {
status: 'ok',
});
await request(app)
.get('/.backstage/health/v1/liveness')
.expect(200, { status: 'ok' });
await request(app).get('/.backstage/health/v1/readiness').expect(200, {
status: 'ok',
});
const beforeShutdownPromise = (lifecycle as any)
.beforeShutdown()
.then(() => {
return (lifecycle as any).shutdown();
});
// Continue accepting requests
await request(app!).get('/test').expect(200, {
status: 'ok',
});
jest.advanceTimersByTime(30000);
await request(app)
.get('/.backstage/health/v1/liveness')
.expect(200, { status: 'ok' });
await request(app!).get('/test').expect(500, {});
// Immediately start failing the readiness health check
await request(app).get('/.backstage/health/v1/readiness').expect(503, {
message: 'Backend has not started yet',
status: 'error',
});
jest.advanceTimersByTime(29999);
// Still accepting requests 1 ms before shutdown
await request(app!).get('/test').expect(200, {
status: 'ok',
});
await request(app)
.get('/.backstage/health/v1/liveness')
.expect(200, { status: 'ok' });
await request(app).get('/.backstage/health/v1/readiness').expect(503, {
message: 'Backend has not started yet',
status: 'error',
});
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' });
await request(app).get('/.backstage/health/v1/readiness').expect(503, {
message: 'Backend has not started yet',
status: 'error',
});
return await expect(beforeShutdownPromise).resolves.toBeUndefined();
});
packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts
+28 -13
View File
@@ -34,6 +34,25 @@ import { DefaultRootHttpRouter } from './DefaultRootHttpRouter';
import { createHealthRouter } from './createHealthRouter';
import { createLifecycleMiddleware } from './createLifecycleMiddleware';
export function getConfigInHumanDuration(
config: RootConfigService,
key: string,
): HumanDuration | undefined {
const value = config.getOptional(key);
if (typeof value === 'undefined') {
return undefined;
}
if (typeof value === 'number') {
return { milliseconds: value };
}
if (typeof value === 'string') {
return {
milliseconds: parseInt(value, 10),
};
}
return readDurationFromConfig(config, { key });
}
/**
* @public
*/
@@ -95,24 +114,20 @@ const rootHttpRouterServiceFactoryWithOptions = (
const healthRouter = createHealthRouter({ config, health });
let startupRequestPauseTimeout: HumanDuration | undefined;
if (config.has('backend.lifecycle.startupRequestPauseTimeout')) {
startupRequestPauseTimeout = readDurationFromConfig(config, {
key: 'backend.lifecycle.startupRequestPauseTimeout',
});
}
const startupRequestPauseTimeout = getConfigInHumanDuration(
config,
'backend.lifecycle.startupRequestPauseTimeout',
);
let shutdownRequestPauseTimeout: HumanDuration | undefined;
if (config.has('backend.lifecycle.shutdownRequestPauseTimeout')) {
shutdownRequestPauseTimeout = readDurationFromConfig(config, {
key: 'backend.lifecycle.shutdownRequestPauseTimeout',
});
}
const shutdownRequestDelayTimeout = getConfigInHumanDuration(
config,
'backend.lifecycle.shutdownRequestDelayTimeout',
);
const lifecycleMiddleware = createLifecycleMiddleware({
lifecycle,
startupRequestPauseTimeout,
shutdownRequestPauseTimeout,
shutdownRequestDelayTimeout,
});
const server = await createHttpServer(