-
Notifications
You must be signed in to change notification settings - Fork 262
Feature/cancellation migration #6014
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
ab73645 to
171de90
Compare
3763876 to
fc5a078
Compare
fc5a078 to
43e12eb
Compare
43e12eb to
34b2334
Compare
34b2334 to
8fa58ae
Compare
8fa58ae to
cdfec82
Compare
|
Previously, pronebird (Andrej Mihajlov) wrote…
it is wrapped when it's spawned via shutdown_tracker
.try_spawn_named_with_shutdown(async move { stream.run().await }, "CoverTrafficStream");which internally does use |
|
Previously, pronebird (Andrej Mihajlov) wrote…
yes, good idea (and now I see exactly what you meant in that zulip message 😅 ) |
|
Previously, pronebird (Andrej Mihajlov) wrote…
I probably just gone in an auto mode and if the previous iteration was using |
|
Previously, pronebird (Andrej Mihajlov) wrote…
similarly to cover traffic stream, shutdown is being handled by the entity who spawns the task |
|
Previously, pronebird (Andrej Mihajlov) wrote…
as before, handled by the client spawning the task : ) |
|
Previously, pronebird (Andrej Mihajlov) wrote…
ibid. |
|
Previously, pronebird (Andrej Mihajlov) wrote…
yeah : ( |
|
Previously, pronebird (Andrej Mihajlov) wrote…
we absolutely should! great catch |
|
Previously, pronebird (Andrej Mihajlov) wrote…
yep. |
|
Previously, pronebird (Andrej Mihajlov) wrote…
fixed |
|
Previously, pronebird (Andrej Mihajlov) wrote…
fixed |
|
Previously, pronebird (Andrej Mihajlov) wrote…
Done. |
|
Previously, pronebird (Andrej Mihajlov) wrote…
cancellation is handled by top pub(crate) async fn start_handling(self)
where
S: AsyncRead + AsyncWrite + Unpin + Send,
R: CryptoRng + RngCore + Send,
{
let remote = self.peer_address;
let shutdown = self.shutdown.clone();
tokio::select! {
_ = shutdown.cancelled() => {
trace!("received cancellation")
}
_ = super::handle_connection(self) => {
debug!("finished connection handler for {remote}")
}
}
}this is because otherwise we'd have to have a lot of cancellation matches all over the place and I think this way is easier. |
|
Previously, pronebird (Andrej Mihajlov) wrote…
similar case to other simple tasks being managed by the client |
8207777 to
cbfa305
Compare
jstuczyn
left a comment
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.
responded and fixed to all issued you pointed out. thanks for checking it out!
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @durch, @neacsu, and @pronebird)
pronebird
left a comment
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.
Reviewable status: 112 of 125 files reviewed, 11 unresolved discussions
common/client-core/src/client/received_buffer.rs line 558 at r2 (raw file):
Previously, jstuczyn (Jędrzej Stuczyński) wrote…
I probably just gone in an auto mode and if the previous iteration was using
wait_with_delay, I'd added wait for cancellation
I didn't bother to comment on all instances because there were too many. Hope those are fixed too.
pronebird
left a comment
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.
Reviewable status: 112 of 125 files reviewed, 8 unresolved discussions
common/client-core/src/client/topology_control/mod.rs line 160 at r2 (raw file):
Previously, jstuczyn (Jędrzej Stuczyński) wrote…
ibid.
I don't know what that means but I assume same as the message before...
pronebird
left a comment
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.
Reviewable status: 112 of 125 files reviewed, 3 unresolved discussions
gateway/src/node/client_handling/websocket/connection_handler/authenticated.rs line 598 at r2 (raw file):
Previously, jstuczyn (Jędrzej Stuczyński) wrote…
cancellation is handled by top
FreshHandler::start_handlingmethod, i.e.:pub(crate) async fn start_handling(self) where S: AsyncRead + AsyncWrite + Unpin + Send, R: CryptoRng + RngCore + Send, { let remote = self.peer_address; let shutdown = self.shutdown.clone(); tokio::select! { _ = shutdown.cancelled() => { trace!("received cancellation") } _ = super::handle_connection(self) => { debug!("finished connection handler for {remote}") } } }this is because otherwise we'd have to have a lot of cancellation matches all over the place and I think this way is easier.
Ok fair. Thanks for sharing. A bit hard to trace the entire execution in a code review.
pronebird
left a comment
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.
Reviewable status: 112 of 125 files reviewed, 1 unresolved discussion
pronebird
left a comment
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.
@pronebird reviewed 13 of 13 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion
this PR moves all of monorepo shutdown watchers from the old
TaskManagerinto the 'new'ShutdownManagerthat internally relies on battle-tested tokio'sCancellationTokenandTaskTrackerinstead of our ownTaskClientcreation.I have tried to incorporate as many spawned tasks as possible within out monorepo into relevant
ShutdownManagertrackers, but that wasn't possible in every case without some time consuming refactoring. for example adding that feature inside theGatewayClientwould require too much refactoring which I reckon is a bit useless given it will be replaced soon enough. similar thing is true for any deeply nested tasks - exposing relevant handles would have been too time consuming.Being a shutdown-only replacement for
TaskManager, it only manages shutdowns, i.e. the broadcastSentStatusmessages are no longer working.TaskManagerfurthermore allows a little bit more flexibility in what signals should trigger the shutdown. it is possible for you to register custom shutdown signals withwith_shutdown(...)method making it, hopefully, more easily to incorporate it into bigger, VPN-like, systems.it also optionally (but enabled by default) triggers shutdown if a panic is detected so hopefully we should no longer be in a situation where a binary is limping around because some internal task has failed without us realising. There is, however, one limitation associated with it. on panic we'll have to wait for the shutdown timeout as the task that has panicked will not stop gracefully (well, duh : )).
As for API changes, I tried to make them as minimal as possible, but some breaking changes were inevitable, this includes using
ShutdownTrackerorShutdownTokenin place ofTaskClientfor anycustom_shutdownbuilder method.What is also worth mentioning,
ShutdownTokenby default will NOT cause global shutdown if dropped, unlike the oldTaskClient. to achieve the same behaviour useShutdownDropGuardinstead (internally uses tokio'sDropGuard)Usage
you use it like you'd use old
TaskManager, e.g.HOWEVER, when possible, it's preferred to spawn them on
ShutdownManager's tracker, so that we could wait for them during shutdown to ensure they stop gracefully:furthermore, if you don't care about your task getting interrupted during cancellation (i.e. it doesn't do any important work that has to complete atomically), you can make
ShutdownManagermanage cancellation signals for you:and if you want to use it to the fullest potential, use
try_spawn_namedandtry_spawn_named_with_shutdownto provide more debugging information.ShutdownTokencan be created in two ways, either via.clone_shutdown_token()or via.child_shutdown_token(). the difference is quite subtle.child_tokencreates a ShutdownToken which will get cancelled whenever the current token gets cancelled. However, if the child gets cancelled, the parent will be unaffacted. In contrast, in a cloned ShutdownToken, cancelling any of the tokens will cause both of them to get cancelled. if in doubt, use the clone variant to have the same old behaviour.for intermediate migration,
ShutdownManagerhas support for creating legacyTaskClient. You'd do it as follows:This change is