-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: Graceful shutdown #1801
base: develop
Are you sure you want to change the base?
feat: Graceful shutdown #1801
Conversation
…tdown_with_new_server
…tdown_with_new_server
77db7fe
to
7062d40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some small suggestions. Looks pretty good 👍
*/ | ||
class Stopper { | ||
boost::asio::io_context ctx_; | ||
std::thread worker_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use async framework here or is this deliberately not using it?
auto connection = wsConnectionBuilder_.connect(yield); | ||
if (not connection) { | ||
handleError(connection.error(), yield); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto connection = wsConnectionBuilder_.connect(yield); | |
if (not connection) { | |
handleError(connection.error(), yield); | |
return; | |
} | |
if (auto connection = wsConnectionBuilder_.connect(yield); connection) { | |
wsConnection_ = std::move(connection).value(); | |
} else { | |
handleError(connection.error(), yield); | |
return; | |
} |
This scopes connection
so that there is no way anyone can use it after we moved from it, even if they wanted to
return; | ||
} | ||
auto const& subscribeCommand = getSubscribeCommandJson(); | ||
auto const writeErrorOpt = wsConnection_->write(subscribeCommand, yield, wsTimeout_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
auto const message = wsConnection_->read(yield, wsTimeout_); | ||
if (not message) { | ||
handleError(message.error(), yield); | ||
return; | ||
} | ||
|
||
auto const handleErrorOpt = handleMessage(message.value()); | ||
if (handleErrorOpt) { | ||
handleError(handleErrorOpt.value(), yield); | ||
return; | ||
} | ||
auto const handleErrorOpt = handleMessage(message.value()); | ||
if (handleErrorOpt) { | ||
handleError(handleErrorOpt.value(), yield); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And more of the same 👍
boost::asio::steady_timer timer{yield.get_executor(), std::chrono::steady_clock::duration::max()}; | ||
onStopReady_.connect([&timer]() { timer.cancel(); }); | ||
boost::system::error_code error; | ||
if (not*stopped_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in this case !*stopped_
is better? 😃
{ | ||
boost::asio::steady_timer timer{yield.get_executor(), std::chrono::steady_clock::duration::max()}; | ||
onStopReady_.connect([&timer]() { timer.cancel(); }); | ||
boost::system::error_code error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do anything at all about this error if it's set?
if (connectionHandler_.isStopping()) { | ||
boost::asio::spawn( | ||
ctx_.get(), | ||
[connection = std::move(connectionExpected).value()](boost::asio::yield_context yield) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Here and other places - we usually pass yield
as auto
in lambdas
); | ||
serverStopped.get(); | ||
balancerStopped.get(); | ||
// etl->stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these left intentionally for future or just forgotten to be removed?
For #442. This is a WIP PR containing current state of graceful shutdown.