Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Handle SIGTERM for graceful shutdowns #16882

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
cfc9fb7
Lint & handle sigterm
AndesKrrrrrrrrrrr Nov 14, 2024
540e92f
Remove eslint disable
AndesKrrrrrrrrrrr Nov 14, 2024
0134886
Add exit hooks to express-server
AndesKrrrrrrrrrrr Nov 22, 2024
535d43d
Add multiple exit hooks for next-server
AndesKrrrrrrrrrrr Nov 22, 2024
471c00b
Add build target
AndesKrrrrrrrrrrr Nov 22, 2024
1d8be1c
Dockerfile defaults and substitutions
AndesKrrrrrrrrrrr Nov 22, 2024
5b949e7
Dockerfile
AndesKrrrrrrrrrrr Nov 22, 2024
b14d1af
Add app/native node_modules to Docker ignores
AndesKrrrrrrrrrrr Nov 22, 2024
eeab818
Updated dockerignores
AndesKrrrrrrrrrrr Nov 22, 2024
991b0de
Add ls -lah, and ARG where needed
AndesKrrrrrrrrrrr Nov 22, 2024
75e19e6
Correct APP_{DIST_,}HOME in compose
AndesKrrrrrrrrrrr Nov 22, 2024
adeb8b1
Add APP_HOME and default APP_DIST_HOME to dist/$APP_HOME
AndesKrrrrrrrrrrr Nov 22, 2024
a5dfc56
Set ARG before use
AndesKrrrrrrrrrrr Nov 22, 2024
6419cc7
Commented and consolidated ARGs
AndesKrrrrrrrrrrr Nov 22, 2024
cade146
No exit 0
AndesKrrrrrrrrrrr Nov 22, 2024
bf72caf
Echo PWD
AndesKrrrrrrrrrrr Nov 22, 2024
bf2cd75
Remove APP_DIST_HOME in compose
AndesKrrrrrrrrrrr Nov 22, 2024
6271954
Cleanup and cache-bustless
AndesKrrrrrrrrrrr Nov 22, 2024
9f48811
Minimal-install packages in dist
AndesKrrrrrrrrrrr Nov 22, 2024
09d859d
Revert non-lib to main
AndesKrrrrrrrrrrr Nov 28, 2024
787f1dc
Remove temporary docker-compose
AndesKrrrrrrrrrrr Nov 28, 2024
75bdd29
Merge branch 'main' into fix/node-sigterm
AndesKrrrrrrrrrrr Nov 28, 2024
97d6a57
feat(core): Gracefully shutdown servers
eirikurn Nov 28, 2024
fdd8a2f
chore: nx format:write update dirty files
andes-it Nov 28, 2024
3fb51a3
Review feedback
eirikurn Nov 29, 2024
4974ecb
Merge branch 'main' into fix/node-sigterm
kodiakhq[bot] Nov 29, 2024
993bc52
Merge branch 'main' into fix/node-sigterm
kodiakhq[bot] Dec 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions libs/infra-express-server/src/lib/infra-express-server.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { setupShutdownHooks } from '@island.is/node-utils'
import express, { Router, Request, Response, NextFunction } from 'express'
import { collectDefaultMetrics, Histogram } from 'prom-client'
import { metricsApp } from './metrics-publisher'
Expand Down Expand Up @@ -41,11 +42,11 @@ export const runServer = ({
return next()
})

app.get('/liveness', function liveness(req, res) {
app.get('/liveness', function liveness(_req, res) {
res.json({ ok: true })
})

app.get('/version', function versionOfCode(req, res) {
app.get('/version', function versionOfCode(_req, res) {
res.json({ version: process.env.REVISION })
})

Expand All @@ -56,7 +57,7 @@ export const runServer = ({

// security middleware
// we should implemente something along the lines of this https://auth0.com/docs/quickstart/backend/nodejs/01-authorization
app.use((req, res, next) => {
app.use((_req, _res, next) => {
// we need to secure all routes by default. OAuth?
next()
eirikurn marked this conversation as resolved.
Show resolved Hide resolved
})
Expand All @@ -66,9 +67,9 @@ export const runServer = ({

app.use(function errorHandler(
err: any, // eslint-disable-line @typescript-eslint/no-explicit-any
req: Request,
_req: Request,
res: Response,
next: NextFunction, // eslint-disable-line
_next: NextFunction,
) {
logger.error(`Status code: ${err.status}, msg: ${err.message}`)
res.status(err.status || 500)
Expand All @@ -84,6 +85,7 @@ export const runServer = ({
const server = app.listen(servicePort, () => {
logger.info(`Listening on port ${servicePort}`)
})
setupShutdownHooks(server)
server.on('error', logger.error)
return server
}
3 changes: 3 additions & 0 deletions libs/infra-nest-server/src/lib/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ export const createApp = async ({
},
)

// Handle signals like SIGINT, SIGTERM and SIGHUP gracefully.
app.enableShutdownHooks()

if (enableVersioning) {
app.enableVersioning()
}
Expand Down
16 changes: 3 additions & 13 deletions libs/infra-next-server/src/lib/bootstrap.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import '@island.is/infra-tracing'
import { setupShutdownHooks } from '@island.is/node-utils'

import createExpressApp, { Express } from 'express'

Expand Down Expand Up @@ -43,33 +44,22 @@ type BootstrapOptions = {
const startServer = (app: Express, port = 4200) => {
const nextPort = parseInt(process.env.PORT || '') || port
const metricsPort = nextPort + 1
app.listen(nextPort, () => {
const server = app.listen(nextPort, () => {
logger.info(
`Next custom server listening at http://localhost:${nextPort}`,
{
context: 'Bootstrap',
},
)
})
setupShutdownHooks(server)
startMetricServer(metricsPort)
}

const setupExitHook = () => {
// Make sure the server doesn't hang after parent process disconnects, eg when
// e2e tests are finished.
if (process.env.NX_INVOKED_BY_RUNNER === 'true') {
process.on('disconnect', () => {
process.exit(0)
})
}
}

export const bootstrap = async (options: BootstrapOptions) => {
const dev = process.env.NODE_ENV !== 'production'
monkeyPatchServerLogging()

setupExitHook()

const expressApp = createExpressApp()

await setupProxy(expressApp, options.proxyConfig, dev)
Expand Down
18 changes: 18 additions & 0 deletions libs/node/utils/.eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"extends": ["../../../.eslintrc.json"],
"ignorePatterns": ["!**/*"],
"overrides": [
{
"files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
"rules": {}
},
{
"files": ["*.ts", "*.tsx"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"rules": {}
}
]
}
24 changes: 24 additions & 0 deletions libs/node/utils/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Node Utils

## `setupShutdownHooks`

The `setupShutdownHooks` function configures a server to properly handle shutdown signals, ensuring a graceful termination process. This is particularly useful for preventing the server from hanging when the parent process disconnects, such as after end-to-end tests complete.

### Parameters

- `server` (Server): An instance of an HTTP server that needs to be shut down gracefully upon receiving termination signals.
- `onShutdown` (Function): Optional callback function that will be executed before the server shuts down. Can be used to perform and wait for other cleanup tasks before the server shuts down.

### Behavior
eirikurn marked this conversation as resolved.
Show resolved Hide resolved

- Listens for system signals indicating process termination: `SIGHUP`, `SIGINT`, and `SIGTERM`.
- When executed by an NX runner (detected by `process.env.NX_INVOKED_BY_RUNNER === 'true'`), it also listens for the `disconnect` event.
- On receiving a termination signal:
- Logs the signal received.
- Closes the server using `server.close()`, awaiting closure before continuing.
- Executes the `onShutdown` callback if provided.
- Logs the successful server shutdown and exits the process with a status code of `0` upon successful shutdown or `1` if an error occurs.

## Running unit tests

Run `nx test node-utils` to execute the unit tests via [Jest](https://jestjs.io).
11 changes: 11 additions & 0 deletions libs/node/utils/jest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/* eslint-disable */
export default {
displayName: 'node-utils',
preset: '../../../jest.preset.js',
testEnvironment: 'node',
transform: {
'^.+\\.[tj]s$': ['ts-jest', { tsconfig: '<rootDir>/tsconfig.spec.json' }],
},
moduleFileExtensions: ['ts', 'js', 'html'],
coverageDirectory: '../../../coverage/libs/node/utils',
}
19 changes: 19 additions & 0 deletions libs/node/utils/project.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"name": "node-utils",
"$schema": "../../../node_modules/nx/schemas/project-schema.json",
"sourceRoot": "libs/node/utils/src",
"projectType": "library",
"tags": [],
"targets": {
"lint": {
"executor": "@nx/eslint:lint"
},
"test": {
"executor": "@nx/jest:jest",
"outputs": ["{workspaceRoot}/coverage/{projectRoot}"],
"options": {
"jestConfig": "libs/node/utils/jest.config.ts"
}
}
}
}
1 change: 1 addition & 0 deletions libs/node/utils/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './lib/setupShutdownHooks'
148 changes: 148 additions & 0 deletions libs/node/utils/src/lib/setupShutdownHooks.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import { setupShutdownHooks } from './setupShutdownHooks'
import { logger } from '@island.is/logging'
import { Server } from 'http'

// Mock the logger
jest.mock('@island.is/logging', () => ({
logger: {
info: jest.fn(),
error: jest.fn(),
},
}))

describe('setupShutdownHooks', () => {
let server: Server
let processExitSpy: jest.SpyInstance
let processOnSpy: jest.SpyInstance

beforeEach(() => {
// Create a mock server
server = {
close: jest.fn((callback) => callback()),
} as unknown as Server

// Spy on process.exit and process.on
processExitSpy = jest
.spyOn(process, 'exit')
.mockImplementation(() => undefined as never)
processOnSpy = jest.spyOn(process, 'on')

// Clear all mocks before each test
jest.clearAllMocks()
})

afterEach(() => {
// Restore all mocks after each test
jest.restoreAllMocks()
})

it('should set up event listeners for termination signals', () => {
// Act
setupShutdownHooks(server)

// Assert
expect(processOnSpy).toHaveBeenCalledWith('SIGHUP', expect.any(Function))
expect(processOnSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function))
expect(processOnSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function))
})

it('should add disconnect event when NX_INVOKED_BY_RUNNER is true', () => {
// Arrange
process.env.NX_INVOKED_BY_RUNNER = 'true'

// Act
setupShutdownHooks(server)

// Assert
expect(processOnSpy).toHaveBeenCalledWith(
'disconnect',
expect.any(Function),
)

// Clean up
process.env.NX_INVOKED_BY_RUNNER = undefined
})

it('should handle shutdown gracefully', async () => {
// Arrange
setupShutdownHooks(server)
const handler = processOnSpy.mock.calls.find(
([event]) => event === 'SIGTERM',
)[1]

// Act
await handler()

// Assert
expect(logger.info).toHaveBeenCalledWith(
'Received SIGTERM, shutting down...',
)
expect(server.close).toHaveBeenCalled()
expect(logger.info).toHaveBeenCalledWith('Server closed successfully')
expect(processExitSpy).toHaveBeenCalledWith(0)
})

it('should handle server close errors', async () => {
// Arrange
const errorServer = {
close: jest.fn(() => {
throw new Error('Server close failed')
}),
} as unknown as Server
setupShutdownHooks(errorServer)
const [[, handler]] = processOnSpy.mock.calls

// Act
await handler()

// Assert
expect(logger.error).toHaveBeenCalledWith(
'Error during shutdown:',
expect.any(Error),
)
expect(processExitSpy).toHaveBeenCalledWith(1)
})

it('should execute onShutdown callback during shutdown', async () => {
// Arrange
const onShutdownMock = jest.fn()
setupShutdownHooks(server, onShutdownMock)
const handler = processOnSpy.mock.calls.find(
([event]) => event === 'SIGTERM',
)[1]

// Act
await handler()

// Assert
expect(onShutdownMock).toHaveBeenCalled()
expect(logger.info).toHaveBeenCalledWith(
'Received SIGTERM, shutting down...',
)
expect(server.close).toHaveBeenCalled()
expect(logger.info).toHaveBeenCalledWith('Server closed successfully')
expect(processExitSpy).toHaveBeenCalledWith(0)
})

it('should handle onShutdown rejection', async () => {
// Arrange
const onShutdownMock = jest
.fn()
.mockRejectedValue(new Error('Shutdown hook failed'))
setupShutdownHooks(server, onShutdownMock)
const handler = processOnSpy.mock.calls.find(
([event]) => event === 'SIGTERM',
)[1]

// Act
await handler()

// Assert
expect(onShutdownMock).toHaveBeenCalled()
expect(logger.error).toHaveBeenCalledWith(
'Error during shutdown:',
expect.any(Error),
)
expect(processExitSpy).toHaveBeenCalledWith(1)
})
})
44 changes: 44 additions & 0 deletions libs/node/utils/src/lib/setupShutdownHooks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { logger } from '@island.is/logging'
import { Server } from 'http'

export const setupShutdownHooks = (
server: Server,
onShutdown?: () => Promise<void>,
) => {
let isShuttingDown = false
const terminationEvents: Array<NodeJS.Signals | 'disconnect'> = [
'SIGHUP',
'SIGINT',
'SIGTERM',
]

// Make sure the server doesn't hang after parent process disconnects, eg when
// e2e tests are finished.
if (process.env.NX_INVOKED_BY_RUNNER === 'true') {
terminationEvents.push('disconnect')
}

const shutdown = async (signal: string) => {
if (isShuttingDown) {
return
}
isShuttingDown = true

try {
logger.info(`Received ${signal}, shutting down...`)
await new Promise((resolve) => server.close(resolve))
if (onShutdown) {
await onShutdown()
}
logger.info('Server closed successfully')
process.exit(0)
} catch (error) {
logger.error('Error during shutdown:', error)
process.exit(1)
}
}

for (const signal of terminationEvents) {
process.on(signal, () => shutdown(signal))
}
}
16 changes: 16 additions & 0 deletions libs/node/utils/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"module": "commonjs"
},
"files": [],
"include": [],
"references": [
{
"path": "./tsconfig.lib.json"
},
{
"path": "./tsconfig.spec.json"
}
]
}
Loading
Loading