Merged
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3601/docs/iroh/ Last updated: 2025-11-04T09:46:33Z |
flub
reviewed
Nov 4, 2025
iroh/src/magicsock.rs
Outdated
| return; | ||
| } | ||
| self.msock.closing.store(true, Ordering::Relaxed); | ||
| self.msock.cancel_transports.cancel(); |
Contributor
There was a problem hiding this comment.
Instead of a new token, would this be done better as a child token from the actor_token that already exists on the line below?
Member
Author
There was a problem hiding this comment.
Yeah, sounds good. Did that.
matheus23
approved these changes
Nov 4, 2025
Member
matheus23
left a comment
There was a problem hiding this comment.
I probably agree having a single close_token instead of two separate cancel_transport and actor_tokens is better, but IMO that's just a nit.
Member
Author
|
Changed to reuse the existing cancel token. |
11 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 6, 2025
## Description Since we merged #3601 we sometimes get ugly (but harmless) error log messages when the endpoint is shutting down: ``` 2025-11-06T11:28:26.721826Z ERROR ep-client:client: iroh::magicsock::transports::relay: relay_recv_channel closed 2025-11-06T11:28:26.721884Z ERROR ep-client:client: iroh_quinn::endpoint: I/O error: connection closed ``` This fixes it: We already have a check in the transports to return `Pending` without polling the actual transports once `Magicsock::is_closed` is `true`. This PR changes it to instead check `Magicsock::is_closing`. The latter is set **after** `Endpoint::wait_idle` completed, but before cancelling the actor and transport tasks. Whereas `is_closed` is set *after* the actor and transport tasks are fully shutdown. When `Endpoint::wait_idle` completes, we do not expect the quinn endpoint to do anything meaningful, so it is fine to stop polling the transports at this time, and thus not get errors from the transports into quinn if they fail (because they are shutting down). ## Breaking Changes <!-- Optional, if there are any breaking changes document them, including how to migrate older code. --> ## Notes & open questions <!-- Any notes, remarks or open questions you have to make about the PR. --> ## Change checklist <!-- Remove any that are not relevant. --> - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented. - [ ] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Since the transport abstraction was introduced, we no longer shut down the relay actor when calling
Endpoint::close. The relay actor is currently only stopped when the last clone of theiroh::Endpointis dropped. This is surprising and wasteful. This PR fixes this so that the relay actor is stopped when callingEndpoint::close.Background: Since introducing the transport abstraction, the relay actor handle is now owned by
Transportswhich is owned byMagicUdpSocketwhich is owned by thequinn::Endpointas itsAsyncUdpSocket. TheAsyncUdpSockettrait has no notification when the endpoint is closing, thus currently the relay transport (and the relay actor, which it owns) is not notified at all when the endpoint is shutting down. Therefore, it keeps running (and potentially reconnecting to the relay or receiving messages) until it is stopped when the last clone of the endpoint is dropped and thus theAbortOnDropHandleowning the relay actor tasks is dropped, finally stopping the relay actor tasks.This PR exposes an existing cancellation token within the relay actor to the magicsocket, allowing it to cancel the relay actors when calling
Endpoint::close.Breaking Changes
Notes & open questions
I tested this by looking at logs after closing an endpoint. Not sure if there is a good way to test this. I'd vote to merge this even without a test because it fixes an issue, unless someone has a good idea on how to test this.
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme