Fix shutdown #2400
Merged
Fix shutdown #2400
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
github-actions
bot
added
examples
arqunis
added
fix
arqunis
approved these changes
Apr 28, 2023
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 18, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 19, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
May 30, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
added a commit
to mkrasnitski/serenity
that referenced
this pull request
Jun 21, 2023
This reverts commit f63e1a4.
mkrasnitski
added a commit
to mkrasnitski/serenity
that referenced
this pull request
Jul 1, 2023
This reverts commit f63e1a4.
Closed
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Jul 6, 2023
This reverts commit f63e1a4.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Sep 21, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
added a commit
to mkrasnitski/serenity
that referenced
this pull request
Sep 21, 2023
This reverts commit f63e1a4.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 17, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
added a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 17, 2023
This reverts commit f63e1a4.
mkrasnitski
pushed a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 24, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
mkrasnitski
added a commit
to mkrasnitski/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit f63e1a4.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit f63e1a4.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
Previous execution path of ShardManager::shutdown: ``` ShardManager::shutdown() sends ShardQueuerMessage::ShutdownShard to ShardQueuer::run() calls ShardQueuer::shutdown() sends ShardRunnerMessage::Shutdown to ShardRunner::recv() calls ShardRunner::handle_rx_value() calls ShardRunner::checked_shutdown() calls ShardManager::shutdown_finished() sends ACK to ShardManager::shutdown(), which can now finally return ``` New execution path: ``` ShardManager::shutdown() calls Shard::shutdown() ``` To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a `shard: Arc<Mutex<Shard>>` field to `ShardRunnerInfo` (`ShardManager` contains a `runners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>`). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store an `Arc<Mutex<Shard>>` too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well. After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple `shutdown` function on `Shard`, which is called directly by `ShardManager`.
arqunis
pushed a commit
to arqunis/serenity
that referenced
this pull request
Oct 24, 2023
This reverts commit f63e1a4.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Calling shutdown was broken:
This PR fixes it.
The following text is taken from a commit description:
Previous execution path of ShardManager::shutdown:
New execution path:
To do that, ShardManager needed direct access to the Shard instance (instead of only sending messages to ShardRunner). So I added a
shard: Arc<Mutex<Shard>>field toShardRunnerInfo(ShardManagercontains arunners: Arc<Mutex<HashMap<ShardId, ShardRunnerInfo>>>). This also meant ShardRunner couldn't own the Shard directly anymore but needed to store anArc<Mutex<Shard>>too and lock it when needed. This makes for a bit of an ugly diff in shard_runner.rs but oh well.After this, I removed shutdown related functions from ShardRunner and ShardQueuer (and the corresponding enum variants from ShardRunnerMessage and ShardQueuerMessage). Instead there's now just a single simple
shutdownfunction onShard, which is called directly byShardManager.