-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
@@ -311,7 +313,7 @@ impl TaskManager { | |||
Box::pin(async move { | |||
join_all(children_shutdowns).await; | |||
completion_future.await; | |||
drop(keep_alive); | |||
keep_alive |
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.
can you explain this change i.e, is drop
not required because it's consumed by the future
?
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.
LGTM expect that I would like an explanation for the removed drop in clean_shutdown
for explicit approval
Yeah, I'm about to add some comment to the code ;) |
|
||
// The keep_alive stuff is holding references to some RPC handles etc. These | ||
// RPC handles spawn their own tokio stuff and that doesn't like to be closed in an | ||
// async context. So, we move the deletion to some other thread. |
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.
how did it work previously? or is this a change in behavior from tokio 0.2 to 1.0?
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.
I think tokio 1.0 can only detect tokio 1.0. So it did not detected the shutdown being async before. And as both used 0.1 or whatever, tokio itself provided an async function.
This is entirely some hackery stuff in here. When we refactor all of this, there should not be any "keep_alive", because this itself is just a hack...
bot merge force |
Trying merge. |
* Upgrade tokio to 1.10 * Fix test runner * Try fix it * Update Cargo.lock * Review feedback * ahhhh * FML * FMT * Fix tests
polkadot companion: paritytech/polkadot#3695