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

feat(core): pass signal when manually invoking close #10775

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

bartversluijs
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

At the moment, when manually invoking the app.close(), no signal can be passed to the shutdown hooks (beforeApplicationShutdown & onApplicationShutdown). This means creating custom graceful shutdown implementations are limited by the fact that something simple as a signal can't be used for handling shutdown features.

Issue Number: N/A

What is the new behavior?

By adding the possibility the pass a signal to the app.close() function, which is still not required, the signal can be used in shutdown hooks, even when custom graceful shutdown implementations are made. Although it's a small feature, it makes creating graceful shutdowns somewhat easier, even if it's for logging only for example.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls
Copy link

Pull Request Test Coverage Report for Build 58763218-68d8-4232-9174-89e899e36f2f

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.404%

Totals Coverage Status
Change from base Build 1506e0bc-b980-484c-bfc0-6bf793b389fb: 0.0%
Covered Lines: 6202
Relevant Lines: 6640

💛 - Coveralls

@micalevisk
Copy link
Member

can you please share a code snippet on how that would look like?

@bartversluijs
Copy link
Contributor Author

Hereby a code snippet as requested for closing the NestJS application with a exit signal:

// Define the custom shutdown signals
const shutdownSignals: Array<NodeJS.Signals> = ['SIGTERM', 'SIGINT', 'SIGABRT'];]

// Listen to those signals
shutdownSignals.forEach((signal) => {
  process.on(signal, async (signal: NodeJS.Signals | undefined, code: number) => {
    // Invoke custom shutdown methods or handlers here, when creating a custom graceful shutdown
    await beforeWorkerShutdown(signal);

    // Then, manually close NestJS and passing the signal
    nestApplication.close(signal);

    // Next, invoke the custom 'onWorkerShutdown' handler.
    await onWorkerShutdown(signal);

    // Exit the process
    process.exit(code);
  });
});

I currently have something like this in place with the cluster package of Node.js. When starting my application, it first creates some workers (web & background), but I have some worker-specific startup and shutdown handlers in place (e.g. a background worker should wait for all queue jobs to complete, but a web worker shouldn't listen to the queue in the first place). But at the moment, I can't pass the signal that is used for a shutdown to the NestJS shutdown handlers. Because it is just a small change and it can have a large benefit, I proposed this change.

@kamilmysliwiec kamilmysliwiec merged commit 785b745 into nestjs:master Feb 1, 2023
@kamilmysliwiec
Copy link
Member

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants