refactor(iroh): Add send queue between relay actor and relays #3026
Conversation
This introduces a new packet send queue between the RelayActor and the various ConnectedRelayActors. Each ConnectedRelayActor will not block while sending them to the actual relay client. This is important for the relay client to become nothing more than a stream and sink directly connected to the network stream in future refactoring. It does not yet change the communication between AsyncUdpSocket::try_send and the RelayActor itself to keep this change smaller. This also should become a separate channel so that the inbox can still handle messages when the relay channel is full. This is a small incremental change that does a few not very nice things, like having some .expect() calls. This is deliberate to keep the changes smaller to review and reason about.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3026/docs/iroh/ Last updated: 2024-12-12T12:19:24Z |
iroh/src/magicsock/relay_actor.rs
Outdated
| relay_send_fut = Either::Left(future::pending()); | ||
| } | ||
| Some(msg) = self.relay_datagrams_send.recv(), if matches!(relay_send_fut, Either::Left(_)) => { | ||
| relay_send_fut = Either::Right(Box::pin(relay_client.send(msg.0, msg.1))); |
There was a problem hiding this comment.
why not just spawn these? or sth..this construction is very hard to follow..
There was a problem hiding this comment.
I really do not want to spawn a task here. The plan is for relay_client to be directly a AsyncWriter, probably wrapped in a Sink (wrapped in some different client of Client struct). So this actor's loop has to learn how to send things to the relay server while not deadlocking itself.
My approach here is to start doing this for just sending packets. The others which send to the relay are Ping and NotePreferred (and kind of GetLocalAddr because it queues up on the same actor inbox right now, but doesn't go all the way to the relay).
If you look at the points at which Ping and GetLocalAddr are being used: this is only when there network has changed and the RelayActor wants to be sure this connection still works. So at that time there's already a potential hiccup happening. I totally want to fix those too, but plan to do that by moving those things into the ConnectedRelayActor instead.
There was a problem hiding this comment.
could you use MaybeFuture to make this a bit simpler?
There was a problem hiding this comment.
I made some changes to MaybeFuture, agreed that this is nicer. PTAL
There was a problem hiding this comment.
much easier to understand (at least for me) now, thanks
| @@ -173,6 +157,41 @@ impl ConnectedRelayActor { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| async fn handle_actor_msg(&mut self, msg: ConnectedRelayMessage) -> bool { | |||
| trace!("tick: inbox: {:?}", msg); | |||
| match msg { | |||
There was a problem hiding this comment.
are you sure all these should be preferred over regular writes? it seems some of these will trigger additional sending, eg Ping and NotePreferred, or amy I missing sth?
There was a problem hiding this comment.
They are not being preferred, the tokio::select! is not biased. The ones that do trigger a send are indeed blocking. I don't like it, but it is not a regression so in the interest of taking small steps I was only improving the SendPacket path.
| #[derive(Debug)] | ||
| pub(crate) struct MaybeFuture<T> { | ||
| /// Future to be polled. | ||
| pub inner: Option<T>, | ||
| } | ||
|
|
||
| impl<T> MaybeFuture<T> { | ||
| /// Creates a [`MaybeFuture`] without an inner future. | ||
| pub(crate) fn none() -> Self { |
There was a problem hiding this comment.
I've leant into the Option sematics and gone with ::none() constructor and .is_some() and .is_none(). This seems to match the naming which is almost OptionalFuture.
The alternative is to lean into collection APIs: ::empty() as constructor and .is_empty() to check (I guess .len() would be it's counterpart but the clippy convention is to use !m.is_empty()).
There was a problem hiding this comment.
I like the Option version
Description
This introduces a new packet send queue between the RelayActor and the various ConnectedRelayActors. Each ConnectedRelayActor will not block while sending them to the actual relay client.
This is important for the relay client to become nothing more than a stream and sink directly connected to the network stream in future refactoring.
It does not yet change the communication between
AsyncUdpSocket::try_send and the RelayActor itself to keep this change smaller. This also should become a separate channel so that the inbox can still handle messages when the relay channel is full.
This is a small incremental change that does a few not very nice things, like having some .expect() calls. This is deliberate to keep the changes smaller to review and reason about.
Breaking Changes
Notes & open questions
Pay most attention to the ConnectedRelayActor and the way it does create a send future without blocking. The exact mechanism will change once it is sending to a stream, but in the mean time this needs to be somewhat reasonable.
There are still tons of bugs in here. But I don't think there are any regressions. All the bugs existed already.
Change checklist