-
Notifications
You must be signed in to change notification settings - Fork 314
feat(iroh)!: introduce transport abstraction #3279
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
Conversation
|
(Note: my "merged main into refactor-transports" commit was a pushed-the-button-by-accident, didn't actually want to do that at that point in time) |
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3279/docs/iroh/ Last updated: 2025-06-11T13:34:43Z |
Cargo.lock
Outdated
| checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" | ||
| dependencies = [ | ||
| "windows-sys 0.59.0", | ||
| "windows-sys 0.48.0", |
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'm a bit confused by this one.
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.
no idea, this changes back in my other PR, cargo seems to resolve this differently in some cases 🤷
iroh/src/magicsock.rs
Outdated
| .expect("disconnected") | ||
| .into_iter() | ||
| .filter_map(|addr| SocketAddr::try_from(addr).ok()) | ||
| .collect(); |
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.
Pretty sure this could be written without allocating to a Vec with a bit more gymnastics... Anyway, I don't mind.
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.
done
| for sender in &self.ip { | ||
| if sender.is_valid_send_addr(addr) { |
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.
This is like super-nitpicky. But I noticed every time when I review that that I have to double-check these lines to figure out what is going on. In a way something like for sender in self.ip.iter().filter(|ip| ip.is_valid_send_addr(addr)) would be clearer.
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 actually find that much harder to read, because the for loop gets split over multiple lines
|
|
||
| match destination { | ||
| #[cfg(wasm_browser)] | ||
| Addr::Ip(..) => return Err(io::Error::other("IP is unsupported in browser")), |
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 wonder if ErrorKind::AddrNotAvailable would be suitable for these errors. Though making it an Other probably makes it clearer where this is coming from, so perhaps this is better anyway.
| metrics: Arc<MagicsockMetrics>, | ||
| ) -> Self { | ||
| // Currently gets updated on manual rebind | ||
| // TODO: update when UdpSocket under the hood rebinds automatically |
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 important is this todo? probably fairly important for ios?
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.
it's important todo eventually, but the world doesn't end if it doesn't make it in this PR
| pub(crate) fn new(config: RelayActorConfig) -> Self { | ||
| let (relay_datagram_send_tx, relay_datagram_send_rx) = mpsc::channel(256); | ||
|
|
||
| let (relay_datagram_recv_tx, relay_datagram_recv_rx) = mpsc::channel(512); |
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 know you're probably copying these values... but how did we end up with the recv queue being twice as long as the send queue? Anyway, something to clean up some other time.
| } | ||
| }; | ||
|
|
||
| buf_out[..dm.buf.len()].copy_from_slice(&dm.buf); |
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.
Long, long in the future I can now see us polling the TCP socket from the relay server directly during this poll, allowing us to avoid this copy. Especially once we don't have any non-quic messages left since then only very few times we'll need to send a non-QUIC message over the relay protocol that should not be passed up.
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.
one can dream
## Description - Use more `Watchable` in more places - Drops STUN, ICMP and hairpin probes, they are not actually needed in our world - restructure report generation - Keep QAD connections open, instead of recreating them on every report - Removes `STUN` functionality from the relay server **Fixes** - #3315 ## Breaking Changes - removes `iroh_relay::protos::stun::StunError` - removes `iroh_relay::server::testing::stun_config` - removes `iroh_relay::protos::stun` - removes `iroh_relay::quic::QuicClient::get_addr_and_latency` - removes `DEFAULT_STUN_PORT` ## Depends on - [x] #3279 - [x] n0-computer/net-tools#27 --------- Co-authored-by: Kasey <[email protected]>
A first step towards the work of #3276
Breaking changes
iroh::watcheris now its own craten0-watcheriroh::endpoint::Endpoint::node_addrnow returnsimpl Watcher<Value = Option<NodeAddr>>iroh::endpoint::Endpoint::home_relaynow returnsimpl Watcher<Value = Vec<RelayUrl>>iroh::endpoint::Endpoint::bound_socketsnow returnsVec<SocketAddr>iroh-quinnis updated to0.14.0Depends on
&mut selfquinn#67