feat(iroh): improve Router shutdown #2978
Conversation
dignifiedquire
changed the title
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2978/docs/iroh/ Last updated: 2024-11-29T10:39:27Z |
iroh/src/protocol.rs
Outdated
| /// Shuts down the accept loop cleanly. | ||
| /// | ||
| /// If some [`ProtocolHandler`] panicked in the accept loop, this will propagate | ||
| /// that panic into the result here. | ||
| pub async fn shutdown(self) -> Result<()> { | ||
| /// | ||
| /// If already shutdown, it just returns `Ok`. |
There was a problem hiding this comment.
| /// If already shutdown, it just returns `Ok`. | |
| /// If already shutdown, it returns `Ok`. |
Text is almost always better off without words like "just" or "only" etc.
There was a problem hiding this comment.
This should probably say something about what it is waiting on. Is it fully shut down when it returns?
iroh/src/protocol.rs
Outdated
| Some(Ok(Err(inner))) => { | ||
| debug!("Task errored: {inner:?}"); | ||
| Some(Ok(Some(()))) => { | ||
| debug!("Task finished"); |
There was a problem hiding this comment.
What is the granularity of tasks here? Basically I'm wondering whether this is too nosy to be on debug level and should rather be on trace level.
There was a problem hiding this comment.
One task per incoming connection
| let protocols = protocols.clone(); | ||
| let token = cancel_token.child_token(); | ||
| join_set.spawn(async move { | ||
| token.run_until_cancelled(handle_connection(incoming, protocols)).await |
There was a problem hiding this comment.
TIL about this function, I would have built this myself!
iroh/src/protocol.rs
Outdated
| debug!("Task finished"); | ||
| } | ||
| Some(Ok(None)) => { | ||
| debug!("Task cancelled"); |
There was a problem hiding this comment.
I'm not sure if we should log this... I mean, it doesn't cost much, but I do think there's potential for needless log pollution.
There was a problem hiding this comment.
switched to trace
iroh/src/protocol.rs
Outdated
| join_set.spawn(async move { | ||
| token.run_until_cancelled(handle_connection(incoming, protocols)).await | ||
| }); |
There was a problem hiding this comment.
| join_set.spawn(async move { | |
| token.run_until_cancelled(handle_connection(incoming, protocols)).await | |
| }); | |
| join_set.spawn(token.run_until_cancelled(handle_connection(incoming, protocols))); |
I'm a sucker for pointfree style. That said, this might not compile. And it might not actually read better to others.
There was a problem hiding this comment.
doesn't compile..
they all need protocols, which is why I haven't done it yet |
|
You could copy the |
|
lets to the test adding in a different PR, as they don't directly test shutdown |
- `shutdown` now takes `&self` - `shutdown` is idempotent, and will immediately return if already shutdown - there is now a `is_shutdown` method to check if the router is already closed - spawned tasks are cancelled through the cancel token as well, to improve shutdown speed
dignifiedquire
force-pushed
the
feat-improve-shutdown
branch
from
adc16ff to
0eaed7d
Compare
Description
shutdownnow takes&selfshutdownis idempotent, and will immediately return if already shutdownis_shutdownmethod to check if the router is already closedArc<Mutex<Option<JoinHandle>>>instead ofShared<JoinHandle>to more cleanly represent shutdownBreaking Changes
iroh::protocol::Router::shutdowntakes&selfinstead ofselfChange checklist