fix: backoff before retry if relay connection terminates #3254
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3254/docs/iroh/ Last updated: 2025-04-07T10:46:34Z |
|
I confirmed manually that this works. To reproduce:
let relay_map = RelayMap::default_from_node("http://localhost:3340".parse()?, 3478);
let endpoint = Endpoint::builder()
.relay_mode(RelayMode::Custom(relay_map))
.bind()
.await?;
With this PR: The backoff then goes up to 5s, and keeps retrying. On |
Frando
changed the title
Frando
changed the title
flub
reviewed
Apr 3, 2025
Frando
force-pushed
the
fix/relay-backoff
branch
from
cf10a7a to
a4e3c89
Compare
Frando
force-pushed
the
fix/relay-backoff
branch
from
ebca398 to
82b14dc
Compare
flub
reviewed
Apr 4, 2025
iroh/src/magicsock/relay_actor.rs
Outdated
|
|
||
| while let Err(err) = self.run_once().await { | ||
| warn!("Relay connection failed: {err:#}"); | ||
| if self.pong_received { |
There was a problem hiding this comment.
My instinct would have been to put this in the error code. Something like:
enum RelayServerError {
Connecting(...),
Handshake(...),
Established(...),
}Then Connecting and Handshake would result in the next backoff value to be used. Established would trigger resetting the backoff timer.
My reason for this is that while this is a bit more complex on the error handling, it is less state on the struct. And that is usually more important I think, because the state on the struct can leak much easier to un-intended places and you needs to keep it in mind much more. To me it increases the cognitive load and complexity much more.
I'm not going to argue too hard for this though. I haven't actually tried to write this code so maybe my suggestion ends up worse anyway.
There was a problem hiding this comment.
I like the suggestion and went ahead and implemented this. Turned out quite OK.
Frando
force-pushed
the
fix/relay-backoff
branch
from
44b7968 to
419d93c
Compare
flub
approved these changes
Apr 7, 2025
|
|
||
| fn sleep(&self, dur: Duration) -> Self::Sleep { | ||
| time::sleep(dur) | ||
| // This is using `impl Future` to return a future without a reference to self. |
There was a problem hiding this comment.
Huh, that's an interesting trick. Must try to remember this.
| .with_min_delay(Duration::from_millis(10)) | ||
| .with_max_delay(Duration::from_secs(16)) | ||
| .without_max_times() | ||
| .build() |
There was a problem hiding this comment.
I noticed that the backon crate does not enable jitter by default. That's bad. We should add with_jitter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently, if the relay server aborts a connection, we would reconnect to the relay server right away. If the relay server happens to always abort the connection right after establishing, we would be retrying in a hot loop without pause. This happens reliably if the relay server aborts the connection because it rejects the client as not authorized.
This PR has two changes:
Healthproblem, which it does for unauthenticated clients right before closing the connection, this is logged withWARNlevelBreaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme