feat(iroh): ping paths and trigger holepunching on networkchange #3796
Conversation
dignifiedquire
changed the title
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3796/docs/iroh/ Last updated: 2026-01-07T20:57:24Z |
| }; | ||
| tx.send(info).ok(); | ||
| } | ||
| RemoteStateMessage::NetworkChange { is_major } => { |
There was a problem hiding this comment.
Can we not do this with the local addr watcher? I don't like having two mechanisms for this, and a watcher seemed nice.
There was a problem hiding this comment.
addr change update happens a lot later, this is why I added this mechanism, and it also happens for potentially other reasons. The addr change will happen eventually when net report finishes, but we don't need to wait for that, to already know we want to recheck our connections in here
There was a problem hiding this comment.
I see. could we more often update our local addrs? i don't think if the interfaces change that we need to wait until we've also run netcheck until we get updates. I'd like to receive them all as quickly as possible, but still only via one API.
There was a problem hiding this comment.
one day, but that would require the much bigger refactor of netreport to be more stream based, which will not happen any time soon
There was a problem hiding this comment.
I'm going to accept this technical debt :)
It's annoyed me for a while though so hopefully this will motivate me to get round to improving this soon.
| } | ||
|
|
||
| if is_major { | ||
| self.trigger_holepunching(); |
There was a problem hiding this comment.
so this code-path trigges holepunching less often than the watcher for the local addrs? but otherwise does the same? it seems more like the is_major detection should be added to trigger_holepunching. And that should also send pings if that is helpful. but i don't think this is helpful yet, i think we need to be able to see the number of in-flight tail-loss probes in select path to benefit from this.
dignifiedquire
force-pushed
the
feat-improve-network-change
branch
from
8fdf74f to
bce59b9
Compare
| } | ||
|
|
||
| fn handle_network_change(&mut self, is_major: bool) { | ||
| for conn in self.connections.values() { |
There was a problem hiding this comment.
I feel like I've forgotten half the things, I'm not really sure why we are sending pings today, because I don't think we can do anything with the resulting behaviour. Each path has it's keep-alive and idle-timeout already set. Sending an explicit ping will not change much: the idle timers are already set and will fire. Sending something now won't change that much I think?
There was a problem hiding this comment.
the idea is to make sure we have new information as early as possible
flub
left a comment
flub
left a comment
There was a problem hiding this comment.
Not sure the pings do anything, but otherwise I'll accept the technical debt being added here as a reasonable tradeoff for now. I'm just a bit sad it's found its way into the this new actor.
| }; | ||
| tx.send(info).ok(); | ||
| } | ||
| RemoteStateMessage::NetworkChange { is_major } => { |
There was a problem hiding this comment.
I'm going to accept this technical debt :)
It's annoyed me for a while though so hopefully this will motivate me to get round to improving this soon.
very fair, I think we can clean this up in some form later |
dignifiedquire
force-pushed
the
feat-improve-network-change
branch
from
bce59b9 to
630344b
Compare
|
I think this PR needs at least two lines of PR description, otherwise it doesn't auto-merge IIUC? |
dignifiedquire
force-pushed
the
feat-improve-network-change
branch
from
62f964d to
3f8bd35
Compare
Depends on n0-computer/quinn#290