From c99a5424474f2feee134f11d209e1d2ff5453a3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 10 Jan 2025 11:34:34 +0100 Subject: [PATCH 1/4] fix: Fix `AsyncUdpSocket` backpressure when connection type is mixed --- iroh/src/magicsock.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/iroh/src/magicsock.rs b/iroh/src/magicsock.rs index 0bef9bd7b7..29f9beaffa 100644 --- a/iroh/src/magicsock.rs +++ b/iroh/src/magicsock.rs @@ -535,15 +535,21 @@ impl MagicSock { } } - let udp_pending = udp_error - .as_ref() - .map(|err| err.kind() == io::ErrorKind::WouldBlock) - .unwrap_or_default(); - let relay_pending = relay_error - .as_ref() - .map(|err| err.kind() == io::ErrorKind::WouldBlock) - .unwrap_or_default(); - if udp_pending && relay_pending { + let udp_pending = udp_addr.is_some() + && udp_error + .as_ref() + .map(|err| err.kind() == io::ErrorKind::WouldBlock) + .unwrap_or_default(); + + let relay_pending = relay_url.is_some() + && relay_error + .as_ref() + .map(|err| err.kind() == io::ErrorKind::WouldBlock) + .unwrap_or_default(); + + let has_path = udp_addr.is_some() || relay_url.is_some(); + + if udp_pending && relay_pending && has_path { // Handle backpressure. Err(io::Error::new(io::ErrorKind::WouldBlock, "pending")) } else { From e2b1db6e90dbea909ecd98f394305fe433bff3fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 10 Jan 2025 12:01:11 +0100 Subject: [PATCH 2/4] Add metrics for when we report `WouldBlock` in `try_send` --- iroh/src/magicsock.rs | 1 + iroh/src/magicsock/metrics.rs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/iroh/src/magicsock.rs b/iroh/src/magicsock.rs index 29f9beaffa..d0a2c8c9f2 100644 --- a/iroh/src/magicsock.rs +++ b/iroh/src/magicsock.rs @@ -551,6 +551,7 @@ impl MagicSock { if udp_pending && relay_pending && has_path { // Handle backpressure. + inc!(MagicsockMetrics, send_pending); Err(io::Error::new(io::ErrorKind::WouldBlock, "pending")) } else { if relay_sent || udp_sent { diff --git a/iroh/src/magicsock/metrics.rs b/iroh/src/magicsock/metrics.rs index 90b5ae9d47..91e2a731da 100644 --- a/iroh/src/magicsock/metrics.rs +++ b/iroh/src/magicsock/metrics.rs @@ -20,6 +20,7 @@ pub struct Metrics { // Data packets (non-disco) pub send_data: Counter, pub send_data_network_down: Counter, + pub send_pending: Counter, pub recv_data_relay: Counter, pub recv_data_ipv4: Counter, pub recv_data_ipv6: Counter, @@ -99,6 +100,7 @@ impl Default for Metrics { // Data packets (non-disco) send_data: Counter::new("send_data"), send_data_network_down: Counter::new("send_data_network_down"), + send_pending: Counter::new("send_data"), recv_data_relay: Counter::new("recv_data_relay"), recv_data_ipv4: Counter::new("recv_data_ipv4"), recv_data_ipv6: Counter::new("recv_data_ipv6"), From 1ae8dad390b941bbdea6390de25890dedff236cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 10 Jan 2025 15:23:34 +0100 Subject: [PATCH 3/4] Actually fix the logic --- iroh/src/magicsock.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/iroh/src/magicsock.rs b/iroh/src/magicsock.rs index d0a2c8c9f2..8cf53c9108 100644 --- a/iroh/src/magicsock.rs +++ b/iroh/src/magicsock.rs @@ -535,21 +535,21 @@ impl MagicSock { } } - let udp_pending = udp_addr.is_some() - && udp_error - .as_ref() - .map(|err| err.kind() == io::ErrorKind::WouldBlock) - .unwrap_or_default(); + let udp_pending = udp_error + .as_ref() + .map(|err| err.kind() == io::ErrorKind::WouldBlock) + .unwrap_or_default(); - let relay_pending = relay_url.is_some() - && relay_error - .as_ref() - .map(|err| err.kind() == io::ErrorKind::WouldBlock) - .unwrap_or_default(); + let relay_pending = relay_error + .as_ref() + .map(|err| err.kind() == io::ErrorKind::WouldBlock) + .unwrap_or_default(); - let has_path = udp_addr.is_some() || relay_url.is_some(); + let udp_path = udp_addr.is_some(); + let relay_path = relay_url.is_some(); + let has_path = udp_path || relay_path; - if udp_pending && relay_pending && has_path { + if (udp_pending || !udp_path) && (relay_pending || !relay_path) && has_path { // Handle backpressure. inc!(MagicsockMetrics, send_pending); Err(io::Error::new(io::ErrorKind::WouldBlock, "pending")) From 988232060cb938c56f45192d54a7b796e4b07b62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Mon, 13 Jan 2025 17:12:51 +0100 Subject: [PATCH 4/4] code review --- iroh/src/magicsock.rs | 13 ++++++++----- iroh/src/magicsock/metrics.rs | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/iroh/src/magicsock.rs b/iroh/src/magicsock.rs index 8cf53c9108..3f849346bd 100644 --- a/iroh/src/magicsock.rs +++ b/iroh/src/magicsock.rs @@ -545,13 +545,16 @@ impl MagicSock { .map(|err| err.kind() == io::ErrorKind::WouldBlock) .unwrap_or_default(); - let udp_path = udp_addr.is_some(); - let relay_path = relay_url.is_some(); - let has_path = udp_path || relay_path; + let would_block = match (&udp_addr, &relay_url) { + (Some(_udp), Some(_relay)) => udp_pending && relay_pending, + (Some(_udp), None) => udp_pending, + (None, Some(_relay)) => relay_pending, + (None, None) => false, + }; - if (udp_pending || !udp_path) && (relay_pending || !relay_path) && has_path { + if would_block { // Handle backpressure. - inc!(MagicsockMetrics, send_pending); + inc!(MagicsockMetrics, send_would_block); Err(io::Error::new(io::ErrorKind::WouldBlock, "pending")) } else { if relay_sent || udp_sent { diff --git a/iroh/src/magicsock/metrics.rs b/iroh/src/magicsock/metrics.rs index 91e2a731da..02a208c6de 100644 --- a/iroh/src/magicsock/metrics.rs +++ b/iroh/src/magicsock/metrics.rs @@ -20,7 +20,7 @@ pub struct Metrics { // Data packets (non-disco) pub send_data: Counter, pub send_data_network_down: Counter, - pub send_pending: Counter, + pub send_would_block: Counter, pub recv_data_relay: Counter, pub recv_data_ipv4: Counter, pub recv_data_ipv6: Counter, @@ -100,7 +100,7 @@ impl Default for Metrics { // Data packets (non-disco) send_data: Counter::new("send_data"), send_data_network_down: Counter::new("send_data_network_down"), - send_pending: Counter::new("send_data"), + send_would_block: Counter::new("send_data"), recv_data_relay: Counter::new("recv_data_relay"), recv_data_ipv4: Counter::new("recv_data_ipv4"), recv_data_ipv6: Counter::new("recv_data_ipv6"),