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

Remove ShardManagerMonitor and other gateway cleanup #2372

Merged
merged 11 commits into from
Apr 8, 2023

Conversation

kangalio
Copy link
Collaborator

@kangalio kangalio commented Apr 4, 2023

The bulk of this PR is removing ShardManagerMonitor. It was just a background task that received ShardManagerMessage's and called the respective ShardManager function. Now, you can just call the respective ShardManager function directly.

The commits about removing ShardManagerMonitor are marked with [SMM]. The other commits do other miscellaneous related clean-up. See commit titles and descriptions for more details.

Largely untested

kangalio added 9 commits April 3, 2023 23:54
dispatch_event was a function that was only ever called inside ShardRunner::run with a ClientEvent argument, which itself is just a single-variant enum that only ever contains a ShardStageUpdateEvent.

So, both ClientEvent and dispatch_event are confusing indirections without no clear purpose and I factored them out by inlining the implementation of dispatch_event into ShardRunner::run
It was a single-variant enum that only ever contained ShardClientMessage. Using ShardClientMessage directly is less confusing
From ShardManagerMonitor to ShardManager

First step in removing ShardManagerMonitor
The message channel for ShardRunner was actually passing around Shard**Client**Message's instead of ShardRunnerMessage's (ShardClientMessage is an enum over ShardRunnerMessage and ShardManagerMessage). That's because ShardRunner hijacked two of ShardManagerMessage's enum variants for its own purpose (Restart and Shutdown). I moved those two to ShardRunnerMessage. And then replaced ShardClientMessage with ShardRunnerMessage for the ShardRunner channel communication.

After that, the only remaining uses of the ShardManagerMessage's enum variants were for communication with ShardManager (bliss!). So I removed the Restart and ShardUpdate variants in favor of executing their logic in ShardManager::{restart, shard_update} directly.
Basically, the only remaining purpose of ShardManagerMonitor was to let Client::start_connection() call it and get the return value back, once ShardManagerMessage::{ShutdownInitiated, ShardInvalidAuthentication, ShardInvalidGatewayIntents, ShardDisallowedGatewayIntents} is received. If we want to inline that code into ShardManager, we need to have a way to get the return value from a ShardManager method call into Client::start_connection. Obvious solution: a channel! So, this commit finally deletes ShardManagerMonitor and replaced the channel to it with a channel for the return value. And Client now stores that channel instead of ShardManagerMonitor and queries it inside start_connection
@github-actions github-actions bot added client Related to the `client` module. gateway Related to the `gateway` module. labels Apr 4, 2023
@mkrasnitski
Copy link
Collaborator

Will review this later today. Just quickly noticed that the docs CI job is failing.

Copy link
Collaborator

@mkrasnitski mkrasnitski left a comment

Choose a reason for hiding this comment

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

I'd say this looks good, aside from a few tiny nits. Untangling the mess that is the gateway code is definitely a good thing (I say this as someone relatively unfamiliar with gateway internals).

Could you provide a sentence or two describing the breaking changes in this PR, for changelog purposes?

src/client/bridge/gateway/shard_manager.rs Outdated Show resolved Hide resolved
src/client/bridge/gateway/mod.rs Outdated Show resolved Hide resolved
src/client/bridge/gateway/shard_queuer.rs Outdated Show resolved Hide resolved
Co-authored-by: Michael Krasnitski <[email protected]>
@kangalio
Copy link
Collaborator Author

kangalio commented Apr 5, 2023

- Removed `InterMessage`, `ShardClientMessage`, `ShardManagerMessage`, `ShardManagerMonitor`, `ShardManagerError`
- Changed `ShardManager::new` return type from `(_, ShardManagerMonitor)` to `(_, UnboundedReceiver<Result<(), GatewayError>)`
  - Instead of calling `ShardManagerMonitor.run()`, you just wait for the return value to arrive via the `UnboundedReceiver`
- Changed `ShardMessenger::new` parameter type from `Sender<InterMessage>` to `Sender<ShardRunnerMessage>`
  - All shard-relevant actions have been centralized into `ShardRunnerMessage`
- Changed `ShardMessenger::send_to_shard` return type from `Result<_, _>` to `()`
  - Errors are now logged in the function directly
- Changed `ShardQueuer`'s and `ShardRunnerOptions`'s field `manage_tx: Sender<ShardManagerMessage>` field to `manager: Arc<Mutex<ShardManager>>`
  - Instead of invoking actions on `ShardManager` by sending messages through a channel, you now invoke methods on `ShardManager` directly

Since you said "a sentence or two":

`ShardManagerMonitor` has been removed in favor of interacting with `ShardManager` directly.

@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 Apr 8, 2023
@arqunis arqunis merged commit 4fcf792 into serenity-rs:next Apr 8, 2023
@kangalio
Copy link
Collaborator Author

kangalio commented Apr 9, 2023

Here is some additional context about the removed types:

Removed:

  • ShardClientMessage
    • used to send commands to a ShardRunner. But we already had a type for that: ShardRunnerMessage. ShardClientMessage existed solely because some shard runner commands ended up in the ShardManagerMessage enum for mysterious reasons, so ShardClientMessage was an enum over either ShardRunnerMessage or ShardManagerMessage. This PR moves all shard runner commands into ShardRunnerMessage, so you should use that type instead now.
  • InterMessage
    • an enum that only ever contained ShardClientMessage. Use that type directly; or rather it's new replacement ShardRunnerMessage.
  • ShardManagerMonitor
    • received ShardManagerMessage's and propagated them to the actual shard manager. This redundancy has been removed and you can instead call methods on ShardManager directly now. All places that previously stored a Sender<ShardManagerMessage> now have a Arc<Mutex<ShardManager>>.
  • ShardManagerMessage
    • see above
  • ShardManagerError
    • duplicated parts of GatewayError and was removed in favor of GatewayError.

Thanks to @tazz4843 for pointing out the lacking explanation

FelixMcFelix added a commit to serenity-rs/songbird that referenced this pull request Apr 12, 2023
Fix issues caused by serenity-rs/serenity#2372 and serenity-rs/serenity#2380.

Additionally, this Boxes the TrySendError from Serenity at Clippy's behest (causing a ~330B Result type).

---------

Co-authored-by: Kyle Simpson <[email protected]>
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request May 18, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request May 30, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Sep 21, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 17, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Oct 24, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
)

The bulk of this commit is removing `ShardManagerMonitor`. It was just a
background task that received `ShardManagerMessage`'s and called the respective
`ShardManager` function. Now, you can just call the respective `ShardManager`
function directly.
FelixMcFelix added a commit to FelixMcFelix/songbird that referenced this pull request Nov 20, 2023
Fix issues caused by serenity-rs/serenity#2372 and serenity-rs/serenity#2380.

Additionally, this Boxes the TrySendError from Serenity at Clippy's behest (causing a ~330B Result type).

---------

Co-authored-by: Kyle Simpson <[email protected]>
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 client Related to the `client` module. enhancement An improvement to Serenity. gateway Related to the `gateway` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants