Skip to content

Conversation

@GnomedDev
Copy link
Member

@GnomedDev GnomedDev commented Apr 21, 2025

This removes the uncallable public functions on ShardManager after the rework, and adds a get_shutdown_trigger function which allows for shutting the shard manager down after it has been mutably borrowed for the lifetime of the bot.

@GnomedDev GnomedDev changed the title Implement ShardManager:;get_shutdown_trigger Implement ShardManager::get_shutdown_trigger Apr 21, 2025
@github-actions github-actions bot added the gateway Related to the `gateway` module. label Apr 21, 2025
@mkrasnitski
Copy link
Collaborator

Doesn't Context::shutdown_all fill the role that this does? I think calling that method should always be possible, right?

@GnomedDev
Copy link
Member Author

Nope, what about shutting down the bot cleanly on control C?

@mkrasnitski
Copy link
Collaborator

Nope, what about shutting down the bot cleanly on control C?

Do you mean shutting down the bot from within a signal handler?

@GnomedDev
Copy link
Member Author

@GnomedDev GnomedDev marked this pull request as ready for review April 27, 2025 17:48
Comment on lines +121 to +129
/// Retrieves a function which can be used to shut down the ShardManager later.
///
/// This function will return `true` if the ShardManager has successfully been
/// notified to shut down, or false if it has already shut down and been dropped.
pub fn get_shutdown_trigger(&self) -> impl FnOnce() -> bool + Send + use<> {
let manager_tx = self.manager_tx.clone();
move || manager_tx.unbounded_send(ShardManagerMessage::Quit(Ok(()))).is_ok()
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think exposing self.manager_tx is maybe a more flexible solution? This function kinda sticks out, and I don't really like the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be more flexible, but it's exposing too much API to public consumers imo, leading to defensive or defective programming inside the currently quite simple ShardManager. This is the bare minimum interface that users (at least I) need, and future refactors/reworks cannot be merged if they cannot replicate this behavior.

@arqunis arqunis added enhancement An improvement to Serenity. breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users labels May 19, 2025
@GnomedDev GnomedDev merged commit e8f5055 into serenity-rs:next May 19, 2025
24 checks passed
@GnomedDev GnomedDev deleted the shutdown-trigger branch May 19, 2025 16:14
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jun 30, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users enhancement An improvement to Serenity. gateway Related to the `gateway` module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants