Skip to content

fix(noq): do a best-effort transmit on connection close #625

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
colinmarc wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(noq): do a best-effort transmit on connection close #625

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions noq/src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,6 +1233,14 @@ impl Drop for ConnectionRef {
// be constructed for the newly opened stream.
conn.implicit_close(&self.shared);
}

// We also flush any pending packets, e.g. the CONNECTION_CLOSE, if we're able to do so
// synchronously. This is best-effort; we won't wait for them to be acked, but we have a
// decent chance of notifying the peer in the case that we're currently shutting down
// ungracefully.
let waker = std::task::Waker::noop();
let mut cx = std::task::Context::from_waker(waker);
let _ = conn.drive_transmit(&mut cx);

@flub flub Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is kind of tempting to do this. But also, there's very little guarantee. E.g. even if a CONNECTION_CLOSE was scheduled for transmit, it is supposed to respect congestion control and might well not be sent (that could be buggy currently, I recall spotting some CONNECTION_CLOSE packets that did not respect congestion control).

I think at the end of the day when ungraceful things happen the remote has to deal with things just timing out. I think the edge-cases for which this would help are not really worth this extra complexity. I'd rather people discover these problems faster in tests if their code is genuinely misbehaving.

So I'm tempted to close this without merging.

@matheus23 matheus23 Jun 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah - I also ran some tests to see if this would actually help.
The idea was to make graceful close work when you just drop(endpoint), but keep the process alive a bit.

I didn't dive into the details of why it didn't quite work, but I didn't get the expected result. At least not when the AsyncUdpSocket implementation is the one it is in iroh (I haven't tested it with a normal UdpSocket in noq).

So yeah - I think this might need a bit more thorough work and looking into how this would actually work.

W.r.t. congestion control I think it is completely fine to not send the CONNECTION_CLOSE if CC doesn't allow it. I expect that in most cases CC will actually allow the close frame, though I admit relying on that behavior is a bad idea.

@colinmarc colinmarc Jun 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

To be clear, I wouldn't really expect it to work with anything other than a normal udp socket. One thing I'm not clear on - with iroh, after migration off the relay, isn't that the case?

@flub flub Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, once iroh selected the direct path it should behave the same as noq with a normal udp socket as far as this PR is concerned.

}
}

Expand Down
Loading