fix(iroh-net): return Poll::Read(Ok(n)) when we have no relay URL or direct addresses in poll_send

[ramfox]

Description

If we have no relay URL or addresses to send transmits for a NodeID in poll_send, what do we do?

Returning Polling::Ready(Err(e)) causes the endpoint to error, which causes all connections to fail.

If we return Polling::Pending (in this case), we have no mechanism for waking the waker once the poll_send is returned. Also, even if we wake up and continue to poll, we will attempt to send the same transmits that we know we cannot send.

If we return Polling::Ready(Ok(0)), we will get into a loop in Quinn that attempts to keep re-sending the same transmits.

However, if we report back to Quinn that we have sent those transmits (by returning Polling::Ready(Ok(n))), then Quinn will move on and attempt to send new transmits. QUIC mechanisms might cause those transmits to be re-sent when we get no ACKs for them, but eventually, the connection will time out.

closes #2226

Change checklist

• Self-review.
• Documentation updates if relevant.
• Tests if relevant.
• All breaking changes documented.

[Frando]

We won't ever call a waker in that case, so I'm wondering if not maybe Poll::Ready(Ok(0)) makes more sense?

Would have to check the quinn source though to see how this behaves for each variant I guess.

Edit: alternatively, if discovery moved into the MagicSock, we could actually store the waker here and wake once we discovered a relay or direct addr. That's more future thoughts though, let's first fix the current "killing the endpoint" situatio.

[ramfox]

We won't ever call a waker in that case, so I'm wondering if not maybe Poll::Ready(Ok(0)) makes more sense?

Thank you, yes, you are right. But I am confused/concerned about one thing. If we never end up sending those messages and keep returning Ok(0), aren't we just going to keep attempting to send those same messages over and over again & never make any progress? Which may block other messages that are trying to be sent on other connections? Is the answer here to just drop the messages for that nodeID on the floor & report that we sent them?

My more aggressive thought would be that we should keep the error since ending up in a state where we have no address for the given node ID should never happen. And when it does happen, people will file bug reports because an error occurred 😂

Edit: alternatively, if discovery moved into the MagicSock, we could actually store the waker here and wake once we discovered a relay or direct addr. That's more future thoughts though, let's first fix the current "killing the endpoint" situation.

I've actually been converted to "We shouldn't move discovery into MagicSock". My reasoning is two-fold:

1. The separation of concerns is actually a nice boundary to help us reason about what is "discovery" and what is "connectivity." It also allows us to communicate with the user. If discovery fails or no addresses exist, we can error before we attempt to connect.
2. If two nodes have previously had a connection and one node changes its addresses, we already have healing mechanisms to communicate those changes using Disco messages and the relay. Also, since we don't prune addresses while the endpoint is running, we should never get into a case where we previously had a connection to a node and then suddenly have no addresses for that node.

I wrote more thoughts about this here: https://www.notion.so/number-zero/Connections-Discovery-and-Migration-9c34bad7dba24f80b20275ac78bdf202

Edits made for grammar

[Frando]

My more aggressive thought would be that we should keep the error since ending up in a state where we have no address for the given node ID should never happen. And when it does happen, people will file bug reports because an error occurred 😂

We can't unfortunately because this error kills the quinn endpoint, not only the connection.

I will check the quinn source what it does if we return Ready(Ok(0)). If that leads to retrying in a loop, then we should likely report the data as sent even though it isn't - this will then make the connection time out after 10s.

[flub]

Also, since we don't prune addresses while the endpoint is running

We do prune unused addresses while running.

[flub]

I think I'm going down the "pretend we sent it, but it got lost somewhere in the internet tubes". With pending you indeed never install a waker.

IIUC correctly Quinn quinn has a per-endpoint send buffer (per connection in 0.11), so returning pending will block everything in that buffer, including other connections.

[ramfox]

We can't unfortunately because this error kills the quinn endpoint, not only the connection.

I know 😈

I will check the quinn source what it does if we return Ready(Ok(0)). If that leads to retrying in a loop, then we should likely report the data as sent even though it isn't - this will then make the connection time out after 10s.

In all seriousness though—it does retry in a loop, so then reporting the data as sent is the way to go. TY!!!

[ramfox]

We do prune unused addresses while running.

Ahh yes, let me rephrase: we never prune to 0. This only matters if we have zero addrs for a node id.