refactor(iroh-net): remove rebinding #2083
Merged
Conversation
This file contains 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
When the network has changed we used to close all the derp connections unconditionally. This passes in the local endpoints information so that the derp http actor will only close those derp connections for which we no longer have the local endpoint. That's a lot less disruptive than always closing them.
We still handle major network changes, but we do not pretend to handle rebinding anymore. There are a few reasons and implications that I'll write up properly tomorrow.
| @@ -1869,15 +1826,6 @@ impl Actor { | |||
| inc!(MagicsockMetrics, update_endpoints); | |||
|
|
|||
| debug!("starting endpoint update ({})", why); | |||
| if self.no_v4_send && !self.inner.is_closed() { | |||
There was a problem hiding this comment.
should we error out here? seems in this case we are quite dead in the water
There was a problem hiding this comment.
Maybe, yes. Probably should be done on receiving the netcheck report in that case to error if there's neither ipv4 or ipv6.
There was a problem hiding this comment.
I'm not sure there's anything for us to do really. Trying to send and receive data will result in an error at which point the application code has to give up on the magicsock like it would have with a normal socket.
The other option is to short-cut this into a fatal error that's propagated in .poll_send() or .poll_recv(), but it achieves just the same.
This keeps the logic more readable instead of having to get concerned with the how.
dignifiedquire
approved these changes
Mar 18, 2024
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
This removes the rebinding functionality of the magicsocket. We did not use this functionality, it was incoherently implemented and thus very confusing what of it was still relevant or not. While there is some theoretical need for rebinding, it is cleaner to remove it all. The main casualty of this is the ability to set a preferred port, to this the answer is to ask to bind to the preferred port to start with, in the EndpointBuilder. The other casualties are some test which now can not be used anymore, they're not a great loss.
We still keep the behaviour of reacting to major network changes by resetting node path state and by pruning derp connections.
Notes & open questions
Change checklist