Graceful shutdown #878
Graceful shutdown #878
Conversation
|
Short description: |
silvestrpredko
force-pushed
the
silvestr/graceful-shutdown
branch
from
21c3949 to
588b90c
Compare
|
i also made a poc for graceful shutdown using cancellation token, you can check it here main...rumqttd-shutdown ! we can compare both approaches and combine good stuff from both :) |
Pull Request Test Coverage Report for Build 9501924124Details
💛 - Coveralls |
I like that handle returns in |
silvestrpredko
force-pushed
the
silvestr/graceful-shutdown
branch
from
eddc878 to
40612fb
Compare
Pull Request Test Coverage Report for Build 9521027502Details
💛 - Coveralls |
silvestrpredko
force-pushed
the
silvestr/graceful-shutdown
branch
from
40612fb to
3222431
Compare
|
Hey @swanandx. I redesigned the approach a little bit. It seems like a good solution. Please check it out.
|
Pull Request Test Coverage Report for Build 9522368042Details
💛 - Coveralls |
|
Hey, @swanandx do you have a chance to look at this PR? |
- Add a `shutdown_rx` parameter to the `start` function to receive shutdown signals. - Handle the shutdown signal in the main loop and break out cleanly. - console.rs: - Add a `shutdown_rx` parameter to the `start` function to receive shutdown signals. - Use `axum::serve` with `with_graceful_shutdown` to handle shutdown signals. - timer.rs: - Add a `shutdown_rx` parameter to the `start` function to receive shutdown signals. - Handle the shutdown signal in the main loop and break out cleanly. - broker.rs: - Pass the `shutdown_rx` to various components like bridge, console, timer, and servers. - Handle shutdown signals in the server's main loop and break out cleanly. - Add a `ShutdownHandler` and `ShutdownDropGuard` for managing shutdown signals. These changes allow for graceful shutdown of the rumqtt broker and its components, ensuring clean termination and resource cleanup when a shutdown signal is received.
silvestrpredko
force-pushed
the
silvestr/graceful-shutdown
branch
from
3222431 to
d39e149
Compare
Pull Request Test Coverage Report for Build 10043003235Details
💛 - Coveralls |
|
@swanandx Just I kindly reminder. Could you please take a look? |
Hey, I sincerely apologise for the delay, but I'm not having any bandwidth as of now or in near future. So this might be delayed 😕 |
|
@swanandx Any updates on this? Is there another member that can take over the review? |
| #[tracing::instrument(skip(self))] | ||
| pub fn start(&mut self) -> Result<(), Error> { | ||
| pub fn start(self) -> Result<BrokerHandler, Error> { |
There was a problem hiding this comment.
This would make me weary about breaking current functionality. Current start function does not return, so those currently using broker start and expecting it to go forever could potentially exit their programs prematurely with this change.
There was a problem hiding this comment.
Unfortunately, when I made this change I didn't think about breaking of current API. In my case, I only need something that will stop the broker when it goes out of execution scope(tests).
I think that it would be better to control how far execution goes by a handler, rather than spawn it and then block execution till it exits. Maybe could be implemented something like that https://github.com/tokio-rs/axum/blob/main/examples/graceful-shutdown/src/main.rs
There was a problem hiding this comment.
No worries. Sorry if I'm stepping on your toes but I submitted #929 last week so I could keep moving on my work. I think an ideal future state would be to make a new start_async() method for the Broker that would look lot what you've built here.
There was a problem hiding this comment.
No problem, I just hope that one of our PRs will be merged.
Type of change
New feature (non-breaking change which adds functionality)
Checklist:
cargo fmtCHANGELOG.mdif it's relevant to the users of the library. If it's not relevant mention why.