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

Graceful shutdown #37

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Graceful shutdown #37

merged 3 commits into from
Mar 25, 2024

Conversation

Victor-N-Suadicani
Copy link
Contributor

@Victor-N-Suadicani Victor-N-Suadicani commented Mar 25, 2024

The app will now gracefully shut down when signalled through a broadcast channel. A convenience function is also added that automatically listens for ctrl-c (non-Unix) or SIGTERM, SIGINT and SIGHUP (Unix).

@Victor-N-Suadicani Victor-N-Suadicani requested review from gorm-issuu and a team March 25, 2024 12:15
Copy link
Contributor

@gorm-issuu gorm-issuu left a comment

Choose a reason for hiding this comment

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

Looks good. Love all the comments, as usual. Only thing I'm wondering about, is why not use the oneshot channel?

/// Shutdown channel. Used to indicate that we should start graceful shutdown.
/// The channel has capacity 1 as we only need to signal once to shutdown.
/// Missing messages on the channel doesn't matter.
shutdown: broadcast::Sender<()>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use a oneshot channel?

Copy link
Contributor Author

@Victor-N-Suadicani Victor-N-Suadicani Mar 25, 2024

Choose a reason for hiding this comment

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

I tried but oneshot is single-producer single-consumer, and we need to both send from multiple places (internally and the user can also) and we need to receive in multiple places (each handler) so the broadcast channel is the only one that satisfies that in tokio.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 makes sense

@Victor-N-Suadicani Victor-N-Suadicani merged commit ded8a84 into main Mar 25, 2024
4 checks passed
@Victor-N-Suadicani Victor-N-Suadicani deleted the graceful_shutdown branch March 25, 2024 13:48
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