feat(iroh-net): allow the underlying UdpSockets to be rebound #2946

dignifiedquire · 2024-11-18T13:11:52Z

Description

In order to handle supsension and exits on mobile. we need to rebind our UDP sockets when they break.

This PR adds the ability to rebind the socket on errors, and does so automatically on known suspension errors for iOS.

When reviewing this, please specifically look at the duration of lock holding, as this is the most sensitive part in this code.

Some references for these errors

TODOs

code cleanup
testing on actual ios apps, to see if this actually fixes the issues
potentially handle port still being in use? this needs some more thoughts

Closes #2939

Breaking Changes

The overall API for netmon::UdpSocket has changed entirely, everything else is the same.

Notes & open questions

I have tried putting this logic higher in the stack, but unfortunately that did not work out.
We might not want to infinitely rebind a socket if the same error happens over and over again, unclear how to handle this.

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-11-18T13:15:11Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2946/docs/iroh/

Last updated: 2024-11-26T17:16:36Z

net-tools/netwatch/src/udp.rs

rklaehn · 2024-11-18T15:45:11Z

net-tools/netwatch/src/udp.rs

+    }
+
+    /// Marks this socket as needing a rebind
+    pub fn mark_broken(&self) {


mark_broken is not called from anywhere.

What's the reason this doesn't just set the option to none? Fear of deadlocks? I was going to look if this is a concern, but since it is not called I could not.

this is all in progress, don't worry about the api too much for now

I would also prefer to go with setting the Option to None since it makes this less vulnerable to toctou races.

Maybe there's something that prevents setting is_broken in a non synchronized way with rebind (which is called after is_broken()) in the higher level calls, but since this is exposed as part of the public i think it would be better to have rebinding (the use part) and is_broken (the check part) as a single locked entity.

We currently use the option only for drop but we could simply do the fancy drop routine when there's actually a socket

Going a bit further we could have a enum like

enum InnerSock { Active(tokio::net::UddSocket), Rebind(std::net::SocketAddr), }

should use a teeny tiny less memory, does not require storing the address when not needed, no need to think about races about whether being broken and actually rebinding are being updated in some incorrect way and uses a single lock

Maybe there's something that prevents setting is_broken in a non synchronized way with rebind (which is called after is_broken()) in the higher level calls, but since this is exposed as part of the public i think it would be better to have rebinding (the use part) and is_broken (the check part) as a single locked entity.

I understand the desire, but mark_broken needs to work without taking a write lock, so it needs to be seperated into an atomic, or its own lock

github-actions · 2024-11-19T14:16:02Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: f5270fb

Arqu · 2024-11-19T16:30:01Z

Current run

test	case	throughput_gbps	throughput_transfer
iroh	1_to_1	0.65	1.05
iroh	1_to_3	1.46	2.14
iroh	1_to_5	2.88	4.96
iroh	1_to_10	4.31	5.62
iroh	2_to_2	1.05	1.61
iroh	2_to_4	2.03	3.16
iroh	2_to_6	3.19	4.86
iroh	2_to_10	4.58	6.48

From the last merged PR

test	case	throughput_gbps	throughput_transfer
iroh	1_to_1	0.55	0.83
iroh	1_to_3	1.27	1.76
iroh	1_to_5	2.12	2.94
iroh	1_to_10	3.70	4.66
iroh	2_to_2	0.56	0.70
iroh	2_to_4	1.13	1.41
iroh	2_to_6	1.99	2.65
iroh	2_to_10	3.34	4.46

divagant-martian

reviewed a small piece (what seemed more critical I think?) in any case I think this is the right approach. What we had before "replacing" some Arc was totally wrong

divagant-martian · 2024-11-20T20:25:39Z

net-tools/netwatch/src/udp.rs

+    }
+
+    /// Marks this socket as needing a rebind
+    pub fn mark_broken(&self) {


I would also prefer to go with setting the Option to None since it makes this less vulnerable to toctou races.

Maybe there's something that prevents setting is_broken in a non synchronized way with rebind (which is called after is_broken()) in the higher level calls, but since this is exposed as part of the public i think it would be better to have rebinding (the use part) and is_broken (the check part) as a single locked entity.

We currently use the option only for drop but we could simply do the fancy drop routine when there's actually a socket

divagant-martian · 2024-11-20T20:34:48Z

net-tools/netwatch/src/udp.rs

+    }
+
+    /// Marks this socket as needing a rebind
+    pub fn mark_broken(&self) {


Going a bit further we could have a enum like

enum InnerSock { Active(tokio::net::UddSocket), Rebind(std::net::SocketAddr), }

should use a teeny tiny less memory, does not require storing the address when not needed, no need to think about races about whether being broken and actually rebinding are being updated in some incorrect way and uses a single lock

net-tools/netwatch/src/udp.rs

divagant-martian · 2024-11-20T20:39:12Z

net-tools/netwatch/src/udp.rs

+        let mut guard = self.socket.write().unwrap();
+        {
+            let socket = guard.take().expect("not yet dropped");
+            drop(socket);


I think we pay here the penalty of the slow drop of the previous socket?

we do, but I this needs to be sync because it is called in poll methods, and I don’t see a way around it

flub · 2024-11-22T09:14:55Z

iroh-net/src/magicsock/udp_conn.rs

+        // update socket state
+        let new_state = self.io.with_socket(|socket| {
+            quinn_udp::UdpSocketState::new(quinn_udp::UdpSockRef::from(socket))
+        })??;


If this fails the UdpConn is left in an unknown and unusable state right? But this takes &self so there is no real indicator that this is broken afterwards.

there is? all the methods will return an error if this happens

flub · 2024-11-22T09:17:51Z

iroh-net/src/magicsock/udp_conn.rs

+                        warn!("failed to rebind socket: {:?}", err);
+                        // TODO: improve error
+                        let err =
+                            std::io::Error::new(std::io::ErrorKind::NotConnected, err.to_string());


I fear that quinn will swallow these errors and try to carry on.

Perhaps rebind should directly return std::io::Error? This branch returns NotConnected while in try_send it returns BrokenPipe. Should these not be consistent?

I fear that quinn will swallow these errors and try to carry on.

really? that would sucks

Perhaps rebind should directly return std::io::Error?

I thought about it, but that would be wrong, because these are send and recv calls, returning an error about binding would be..odd. Which is why I want to return an error which means that the connection is broken.

flub · 2024-11-22T09:20:43Z

iroh-net/src/magicsock/udp_conn.rs

+                            count = meta.len / meta.stride,
+                            dst = %meta.dst_ip.map(|x| x.to_string()).unwrap_or_default(),
+                            "UDP recv"
+                        );


top marks for logging call! :)

net-tools/netwatch/src/udp.rs

flub · 2024-11-22T09:28:49Z

net-tools/netwatch/src/udp.rs

+        let guard = self.socket.read().unwrap();
+        let Some(socket) = guard.as_ref() else {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::BrokenPipe,


how was this error chosen? i mean, why is this one the right error? might be worth documenting in the code.

it was the closest to me that indicates that the connection is broken, would love to hear better suggestions.

net-tools/netwatch/src/udp.rs

flub · 2024-11-26T12:06:52Z

net-tools/netwatch/src/udp.rs

+        let mut guard = self.socket.write().unwrap();
+        {
+            let Some(socket) = guard.take() else {
+                bail!("cannot rebind closed socket");


should state the reason: socket already closed or no socket to rebind or something like that.

net-tools/netwatch/src/udp.rs

flub · 2024-11-26T12:17:19Z

net-tools/netwatch/src/udp.rs

+                    }
+                    Err(err) => {
+                        warn!("failed to rebind socket: {:?}", err);
+                        // TODO: improve error


What still needs to be added to the error? I think this should be resolved before merging as otherwise it'll be here forever.

agreed, I am just not sure what the best way here is

net-tools/netwatch/src/udp.rs

iroh-net/src/magicsock/udp_conn.rs

dignifiedquire · 2024-11-26T13:59:14Z

@flub cleaned up and dryed the code a bit, based on your comments

to deal with broken pipes

iroh-net/src/magicsock/udp_conn.rs

flub

There are still unresolved comments, but essentially this looks fine I think.

flub · 2024-11-26T15:55:13Z

net-tools/netwatch/src/udp.rs

+}
+
+#[derive(Debug)]
+enum SocketState {


net-tools/netwatch/src/udp.rs

matheus23

Yay this is nice
(Also make the formatter happy though: cargo make format)

Arqu · 2024-11-26T17:12:30Z

Current branch last run:

test	case	throughput_gbps	throughput_transfer
iroh	1_to_1	1.37	1.37
iroh	1_to_3	4.42	4.42
iroh	1_to_5	7.20	7.21
iroh	1_to_10	11.86	11.88
iroh	2_to_2	3.03	3.04
iroh	2_to_4	5.89	5.90
iroh	2_to_6	8.38	8.39
iroh	2_to_10	13.55	13.57

Current main:

test	case	throughput_gbps	throughput_transfer
iroh	1_to_1	1.36	1.36
iroh	1_to_3	4.42	4.43
iroh	1_to_5	6.89	6.90
iroh	1_to_10	11.33	11.35
iroh	2_to_2	2.72	2.72
iroh	2_to_4	5.54	5.55
iroh	2_to_6	7.87	7.88
iroh	2_to_10	13.32	13.35

Seems like its about the same or slight improvement.

flub reviewed Nov 18, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Show resolved Hide resolved

flub reviewed Nov 18, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Outdated Show resolved Hide resolved

flub reviewed Nov 18, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Outdated Show resolved Hide resolved

rklaehn reviewed Nov 18, 2024

View reviewed changes

dignifiedquire force-pushed the feat-rebinding-socket branch from 0323e98 to 16d1e39 Compare November 19, 2024 12:33

dignifiedquire added this to the v0.29.0 milestone Nov 20, 2024

dignifiedquire force-pushed the feat-rebinding-socket branch from d90f32a to 8d574ed Compare November 20, 2024 15:49

divagant-martian reviewed Nov 20, 2024

View reviewed changes

flub reviewed Nov 22, 2024

View reviewed changes

dignifiedquire force-pushed the feat-rebinding-socket branch from 0b5966c to e9bcb40 Compare November 22, 2024 10:11

matheus23 self-assigned this Nov 25, 2024

dignifiedquire changed the title ~~[WIP] feat(iroh-net): allow the underlying UdpSockets to be rebound~~ feat(iroh-net): allow the underlying UdpSockets to be rebound Nov 25, 2024

dignifiedquire marked this pull request as ready for review November 25, 2024 18:01

flub reviewed Nov 26, 2024

View reviewed changes

dignifiedquire added 12 commits November 26, 2024 14:59

wip: rebinding socket functionality

a5a7526

to deal with broken pipes

wip test

e435086

simplify iopoller

c343e0e

fixup basic test and impl

df8c966

cleanup

02ff112

reduce cloning and add rebind method to UdpConn

175d69c

locking is hard

2d34517

start handling read errors

082e948

fix: hold locks for less time

e90d182

move rebind call

c814284

handle read and write erros

9441b27

handle errors in udp_conn as well

c09f6dd

matheus23 and others added 7 commits November 26, 2024 14:59

Simpler regression test case

d24ab72

use atomic waker, and register properly

6a0212c

fixup

95f29c8

some more cleanup

6194890

Don't block when locking in poll

724b92e

Use socket2::SockRef for connect instead of holding a write lock

003294e

cleanup socket code

605fd16

dignifiedquire force-pushed the feat-rebinding-socket branch from f518925 to 605fd16 Compare November 26, 2024 13:59

less warning

bd572c8

matheus23 removed their assignment Nov 26, 2024

matheus23 reviewed Nov 26, 2024

View reviewed changes

iroh-net/src/magicsock/udp_conn.rs Outdated Show resolved Hide resolved

flub approved these changes Nov 26, 2024

View reviewed changes

matheus23 reviewed Nov 26, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Outdated Show resolved Hide resolved

matheus23 reviewed Nov 26, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Outdated Show resolved Hide resolved

matheus23 reviewed Nov 26, 2024

View reviewed changes

net-tools/netwatch/src/udp.rs Outdated Show resolved Hide resolved

dignifiedquire added 3 commits November 26, 2024 17:17

store last socket state

db5da67

update atomic ops

b6a4299

happy clippy

f56e95d

matheus23 approved these changes Nov 26, 2024

View reviewed changes

dignifiedquire added 3 commits November 26, 2024 17:40

normalize errors to std::io

62cbe4d

fmt..

3c06274

...

cc8d286

flub approved these changes Nov 26, 2024

View reviewed changes

happy clippy

23b4eb7

dignifiedquire enabled auto-merge November 26, 2024 17:14

dignifiedquire added this pull request to the merge queue Nov 26, 2024

Merged via the queue into main with commit cc9e4e6 Nov 26, 2024
26 of 27 checks passed

dignifiedquire deleted the feat-rebinding-socket branch November 28, 2024 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-net): allow the underlying UdpSockets to be rebound #2946

feat(iroh-net): allow the underlying UdpSockets to be rebound #2946

dignifiedquire commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

rklaehn Nov 18, 2024

dignifiedquire Nov 18, 2024

divagant-martian Nov 20, 2024

divagant-martian Nov 20, 2024

dignifiedquire Nov 21, 2024

github-actions bot commented Nov 19, 2024 •

edited

Loading

Arqu commented Nov 19, 2024 •

edited

Loading

divagant-martian left a comment

divagant-martian Nov 20, 2024

divagant-martian Nov 20, 2024

divagant-martian Nov 20, 2024

dignifiedquire Nov 20, 2024

flub Nov 22, 2024

dignifiedquire Nov 25, 2024

flub Nov 22, 2024

flub Nov 22, 2024

dignifiedquire Nov 25, 2024

dignifiedquire Nov 25, 2024

flub Nov 22, 2024

flub Nov 22, 2024

dignifiedquire Nov 25, 2024

flub Nov 26, 2024

flub Nov 26, 2024

dignifiedquire Nov 26, 2024

dignifiedquire commented Nov 26, 2024

flub left a comment

flub Nov 26, 2024

matheus23 left a comment

Arqu commented Nov 26, 2024

feat(iroh-net): allow the underlying UdpSockets to be rebound #2946

feat(iroh-net): allow the underlying UdpSockets to be rebound #2946

Conversation

dignifiedquire commented Nov 18, 2024 • edited Loading

Description

TODOs

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2024 • edited Loading

Arqu commented Nov 19, 2024 • edited Loading

divagant-martian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Nov 26, 2024

flub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheus23 left a comment

Choose a reason for hiding this comment

Arqu commented Nov 26, 2024

dignifiedquire commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 19, 2024 •

edited

Loading

Arqu commented Nov 19, 2024 •

edited

Loading