Skip to content

fix: shutdown block #3762

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.

Sign up for GitHub

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

Open
Arqu wants to merge 3 commits into
base: feat-multipath
Choose a base branch
from
Open

fix: shutdown block #3762

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4c0c14
fix: fix shutdown block
Arqu Dec 10, 2025
e13a4fe
bump quinn
Arqu Dec 10, 2025
0e90796
bump quinn
Arqu Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ unused-async = "warn"


[patch.crates-io]
iroh-quinn = { git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
iroh-quinn-proto = { git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
iroh-quinn-udp = { git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
iroh-quinn = { git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
iroh-quinn-proto = { git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
iroh-quinn-udp = { git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }

netwatch = { git = "https://github.com/n0-computer/net-tools", branch = "main" }

Expand Down
4 changes: 2 additions & 2 deletions iroh-relay/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ postcard = { version = "1", default-features = false, features = [
"use-std",
"experimental-derive",
] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main-iroh", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
rand = "0.9.2"
reqwest = { version = "0.12", default-features = false, features = [
"rustls-tls",
Expand Down
8 changes: 4 additions & 4 deletions iroh/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ n0-watcher = "0.6"
netwatch = { version = "0.12" }
pin-project = "1"
pkarr = { version = "5", default-features = false, features = ["relays"] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main-iroh", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
quinn-udp = { package = "iroh-quinn-udp", git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
quinn-udp = { package = "iroh-quinn-udp", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
rand = "0.9.2"
reqwest = { version = "0.12", default-features = false, features = [
"rustls-tls",
Expand Down Expand Up @@ -82,7 +82,7 @@ hickory-resolver = "0.25.1"
igd-next = { version = "0.16", features = ["aio_tokio"] }
netdev = { version = "0.39.0" }
portmapper = { version = "0.12", default-features = false }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main-iroh", default-features = false, features = ["runtime-tokio", "rustls-ring"] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2", default-features = false, features = ["runtime-tokio", "rustls-ring"] }
tokio = { version = "1", features = [
"io-util",
"macros",
Expand Down
2 changes: 1 addition & 1 deletion iroh/bench/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ iroh = { path = "..", default-features = false }
iroh-metrics = { version = "0.37", optional = true }
n0-future = "0.3.0"
n0-error = "0.1.0"
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main-iroh" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "arqu/fix_cid_panic2" }
rand = "0.9.2"
rcgen = "0.14"
rustls = { version = "0.23.33", default-features = false, features = ["ring"] }
Expand Down
4 changes: 2 additions & 2 deletions iroh/examples/transfer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ async fn provide(endpoint: Endpoint, size: u64) -> Result<()> {

// We sent the last message, so wait for the client to close the connection once
// it received this message.
let res = tokio::time::timeout(Duration::from_secs(3), async move {
let res = tokio::time::timeout(Duration::from_secs(4), async move {
Copy link
Collaborator Author

@Arqu Arqu Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are needed because our default connection close time is a hair more than 3s (default PTO 333ms, with max calculating to 1.0x seconds and close time is 3x that)

let closed = conn.closed().await;
let remote = endpoint_id.fmt_short();
if !matches!(closed, ConnectionError::ApplicationClosed(_)) {
Expand Down Expand Up @@ -422,7 +422,7 @@ async fn fetch(endpoint: Endpoint, remote_addr: EndpointAddr) -> Result<()> {

// We received the last message: close all connections and allow for the close
// message to be sent.
tokio::time::timeout(Duration::from_secs(3), endpoint.close())
tokio::time::timeout(Duration::from_secs(4), endpoint.close())
.await
.anyerr()?;

Expand Down
6 changes: 5 additions & 1 deletion iroh/src/magicsock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,11 @@ impl Handle {
#[instrument(skip_all)]
pub(crate) async fn close(&self) {
trace!(me = ?self.public_key, "magicsock closing...");

// Set closing flag BEFORE wait_idle() to prevent new net_report runs
// from creating QAD connections while we're draining existing ones.
self.msock.closing.store(true, Ordering::Relaxed);

Copy link
Collaborator Author

@Arqu Arqu Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have in flight conns if we close later which prevents closing at all and makes wait idle choke.

// Initiate closing all connections, and refuse future connections.
self.endpoint.close(0u16.into(), b"");

Expand All @@ -1002,7 +1007,6 @@ impl Handle {
if self.msock.is_closed() {
return;
}
self.msock.closing.store(true, Ordering::Relaxed);
self.shutdown_token.cancel();

// MutexGuard is not held across await points
Expand Down
11 changes: 10 additions & 1 deletion iroh/src/magicsock/transports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,7 @@ impl TransportsSender {
src: Option<IpAddr>,
transmit: &Transmit<'_>,
) -> Poll<io::Result<()>> {
let mut found_transport = false;
Copy link
Contributor

@flub flub Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I were to nitpick I'd make this let mut fallback_ret = Poll::Ready(Ok(())); and let each transport set this to pending when they hit a pending. But there are many ways to do this and it's subjective and maybe even a nicer way exists.

match dst {
#[cfg(wasm_browser)]
Addr::Ip(..) => {
Expand All @@ -503,6 +504,7 @@ impl TransportsSender {
Addr::Ip(addr) => {
for sender in &mut self.ip {
if sender.is_valid_send_addr(addr) {
found_transport = true;
match Pin::new(sender).poll_send(cx, *addr, src, transmit) {
Poll::Pending => {}
Poll::Ready(res) => {
Expand All @@ -519,6 +521,7 @@ impl TransportsSender {
Addr::Relay(url, endpoint_id) => {
for sender in &mut self.relay {
if sender.is_valid_send_addr(url, endpoint_id) {
found_transport = true;
match sender.poll_send(cx, url.clone(), *endpoint_id, transmit) {
Poll::Pending => {}
Poll::Ready(res) => {
Expand All @@ -533,7 +536,13 @@ impl TransportsSender {
}
}
}
Poll::Pending
if found_transport {
Copy link
Collaborator Author

@Arqu Arqu Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Netsim continued to choke on this for the --relay-only tests because apparently when the other transport is disabled it gets stuck here pending forever.

Poll::Pending
} else {
Poll::Ready(Err(io::Error::other(format!(
"no transport available for {dst:?}"
))))
Comment on lines +542 to +544
Copy link
Contributor

@flub flub Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a permanent error? IIRC in the past we've always decided to return Poll::Ready(Ok(())) and let the the QUIC stack use loss detection to figure out this is lost.

The quinn-udp philosophy is that only permanent errors should be errors. That is: this socket will never ever be usable again and won't ever be able to send a datagram and needs to be re-bound to get any progress.

And I think that this is not the case here: if you have one transport you can still send on that transport. So I think this should probably not be an error but rather a Poll::Ready(Ok(())).

It would still be helpful to log this though. But if we naively log this we'd end up with flooding the log. IIRC quinn-udp itself has a similar situation where they add a bit of state to the socket so that they only emit that log once every 60 seconds or something like that. Could we do the same? I'd vote debug-level logging for this.

Copy link
Contributor

@dignifiedquire dignifiedquire Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a permanent error?

yes, transports are not configurable at runtime (for now) so if we have no transport available that works can error out

buut, I don't know why we would ever hit this in our current tests

Copy link
Contributor

@flub flub Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I disagree. You could be sending a datagram for a disabled transport which would result in an error. But later you could send a datagram to an enabled transport, and that should still succeed. So it's a transient error I think.

Copy link
Contributor

@dignifiedquire dignifiedquire Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transports can't be enabled or disabled currently, this could be the case in the future, but right now that is not possible

}
}
}

Expand Down
Loading