-
Notifications
You must be signed in to change notification settings - Fork 320
Simplify the idle timeout logic & don't short-circuit in handle_msg_send_datagram
#3678
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
Simplify the idle timeout logic & don't short-circuit in handle_msg_send_datagram
#3678
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3678/docs/iroh/ Last updated: 2025-11-18T17:19:44Z |
43d6cc7 to
065f81d
Compare
065f81d to
ca37fd1
Compare
| let idle_timeout = MaybeFuture::None; | ||
| tokio::pin!(idle_timeout); | ||
| let idle_timeout = time::sleep(ACTOR_MAX_IDLE_TIMEOUT); | ||
| n0_future::pin!(idle_timeout); |
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 about using the std lib? https://doc.rust-lang.org/std/pin/macro.pin.html
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.
Fair. I wanted to match the rest of the code's style.
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.
Err.. Seems like that macro actually does something different. It doesn't seem to be a drop-in replacement for neither tokio::pin! nor n0_future::pin! in this situation.
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.
let mut idle_timeout = std::pin::pin!(time::sleep(...)) is the std version. I don't really know which I prefer, this version existed in n0_future so I assumed we liked it for some reason?
handle_msg_send_datagram
flub
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.
I prefer this version too.
|
Not sure why netsim fails now :/ I've tried to spelunking in the logs, but couldn't find anything easily. I thought we'd fixed all netsim issues in this stack of PRs. :/ TBD. |
|
Weird, it seems to complete successfully yet the logs say the process completed with error code 1. @Arqu any ideas? |
|
This log seems to come from chuck's help |
899df00
into
Frando/mp-fix-remote-state-actor-loop

Intended for merging into #3676
idle_timeoutalways be atime::Sleepthat is.resetonce there is any activity. I find this much easier to reason about.handle_msg_send_datagramshort-circuits when sending on one path fails. Instead, it should try all paths, even if some of them fail.