Wait on all background tasks to finish (or abort) #612
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
Grrr, as usual nothing is easy with tokio. Drafting again until I figured it out. |
.. as tokio tends to panic if dropping a runtime in an async context and we're not super careful. Here, we add some test coverage for this edge case in Rust tests.
Previously, individual chain sources would hold references to the `Broadcaster` to acquire the broadcast queue. Here, we move this to `ChainSource`, which allows us to handle the queue in a single place, while the individual chain sources will deal with the actual packages only.
Rather than looping in the `spawn` method directly, we move the loop to a refactored `continuously_process_broadcast_queue` method on `ChainSource`, which also allows us to react on the stop signal if we're polling `recv`.
tnull
force-pushed
the
2025-08-shutdown-wait-on-all-tasks
branch
from
ec5229c to
d4e7727
Compare
|
|
tnull
force-pushed
the
2025-08-shutdown-wait-on-all-tasks
branch
from
d4e7727 to
bc6ea9b
Compare
tnull
force-pushed
the
2025-08-shutdown-wait-on-all-tasks
branch
from
bc6ea9b to
7ec7f29
Compare
|
Rebased on main again with a slight fix to avoid the previous tokio-panic-on-drop. Should be good to land independently now. |
tnull
force-pushed
the
2025-08-shutdown-wait-on-all-tasks
branch
2 times, most recently
from
2bf7920 to
29e02a0
Compare
Previously, we'd only wait for the background processor tasks to successfully finish. It turned out that this could lead to races when the other background tasks took too long to shutdown. Here, we attempt to wait on all background tasks shutting down for a bit, before moving on.
tnull
force-pushed
the
2025-08-shutdown-wait-on-all-tasks
branch
from
29e02a0 to
17a45dd
Compare
| let runtime_2 = Arc::clone(&runtime); | ||
| tasks.abort_all(); | ||
| tokio::task::block_in_place(move || { | ||
| runtime_2.block_on(async { while let Some(_) = tasks.join_next().await {} }) |
There was a problem hiding this comment.
nit: there's a join_all we can call, I think.
There was a problem hiding this comment.
No, I avoided using join_all in this PR as it may panic if any of the tasks return an error. From the tokio docs:
If any tasks on the JoinSet fail with an JoinError, then this call to join_all will panic and all remaining tasks on the JoinSet are cancelled.
Very slim chances to ever hit this, but I'd still prefer not to panic ever.
There was a problem hiding this comment.
Ah, missed that, indeed, we should avoid it.
Fixes #611.
Possibly fixes #586.
Previously, we'd only wait for the background processor tasks to successfully finish. It turned out that this could lead to races when the other background tasks took too long to shutdown. Here, we attempt to wait on all background tasks shutting down for a bit, before moving on.
To allow clean exit of the transaction broadcasting task, we also include two refactoring commits (which are also a nice cleanup on their own right though).