Skip to content

Conversation

@BelgianNoise
Copy link

Description

I was having trouble generating code coverage for my tests with nyc.
Took me some time but I found out that nyc requires the process it spawn to gracefully exit.
I was not completely sure what that meant, so I had to do some digging.
This PR adds handling of signals to gracefully shut down the process.

Changes

  • Added process signal listeners to gracefully shut down the server

Checks

  • Project Builds
  • Project passes tests and checks
  • Updated documentation accordingly

Additional information

I added a cleanup() function that is currently empty, question is: does Booster have dedicated functions to close down the server, clean data, close connections ?
Didn't add docs on the functions as I find them self explanatory.

This fixes the issue for me locally, but I am not entirely sure if this is the way to go. (approach + location in Booster code itself)

Does this require any documentation to be updated ?

@what-the-diff
Copy link

what-the-diff bot commented Apr 18, 2025

PR Summary

  • Introduction of Shutdown Signal Handlers
    A new function named registerShutdownSignalHandlers() has been added. Its purpose is to manage shutdown signals, these are alerts that inform the system when it needs to stop the program. This is crucial to maintain the program's integrity and prevent abrupt termination that might result in data loss or corruption.

  • Graceful Shutdown Implementation
    We've added a function called shutDownGracefully(signal: string). This is used to note down the received signals and execute any cleanup operations required. Cleanup operations refer to the tasks completed by software when it receives a command to stop, to ensure all data and processes are correctly closed down.

  • Placeholder for Future Cleanup Tasks
    The cleanup() function added is currently a placeholder. This means that while it does not currently perform any operations, it is meant to contain cleanup tasks in the future. Cleanup tasks are actions to be taken prior to shutting down a system to ensure no ongoing processes are interrupted, and no data is lost.

  • Integration of Shutdown Handlers
    The startup method of the application (Booster.start()) will now call the registerShutdownSignalHandlers() function. This ensures that the shutdown handlers are active from the moment the project starts running, providing a safety net for proper system termination.

}

function cleanup(): void {
// Does Booster have dedicated clean up to do ?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BelgianNoise Booster currently does not have a dedicated cleanup upon shutdown, but I think it would be beneficial to implement one. It should probably:

  • Stop accepting incoming GraphQL connections/requests and close WebSocket connections.
  • Close connections to DB and read model stores (this would have to be provider-specific).
  • Close any other connections, such as to Event Hubs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your feedback, we have done some more internal debugging and found that it was an issue in our provider that was causing issues with the child process shenanigans & nyc.

That being said, this could still be a nice addition to the core package imo. However I won't be able to focus on it right now as I would need to find time outside working hours to make these changes and get more familiar with the code :)

I am stil eager to go deeper into the code and add the things you mentioned above, but it won't be in teh short term.
I will revert my PR back to a draft and I am leaving it up to you if you would rather see it closed for now.

(Feel free to hijack this PR if desired)

Edit: I do not have access to revert back to draft :)

(Oops, commented with diff account earlier)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BelgianNoise I think we can go ahead with this addition and eventually implement the cleanup method in future PRs :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants