fix(iroh): remove quinn::Endpoint::wait_idle from iroh::Endpoint::close process (#3165)

## Description

`quinn::Endpoint::wait_idle` adds a minimum 3 second closing time to
`iroh::Endpoint::close` if you have any connections that have not closed
gracefully.

While we *want* users to close connections gracefully, we should also
not be forcing users to wait 3 seconds to close the `iroh::Endpoint` if
a connection has "gone wrong" if they don't want to.

So instead, we are taking out `quinn::Endpoint::wait_idle` and adding
more context for how to close a connection gracefully.

## Notes & open questions

Before 1.0 we will be re-visiting closing to make sure the APIs make
sense.

## Change checklist

- [x] Self-review.
- [x] 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.

---------

Co-authored-by: “ramfox” <“[email protected]”>

Author: ramfox

iroh/src/endpoint.rs

@@ -991,10 +991,13 @@ impl Endpoint {
     /// of `0` and an empty reason.  Though it is best practice to close those
     /// explicitly before with a custom error code and reason.
     ///
-    /// It will then make a best effort to wait for all close notifications to be
-    /// acknowledged by the peers, re-transmitting them if needed. This ensures the
-    /// peers are aware of the closed connections instead of having to wait for a timeout
-    /// on the connection. Once all connections are closed or timed out, the magic socket is closed.
+    /// This will not wait for the [`quinn::Endpoint`] to drain connections.
+    ///
+    /// To ensure no data is lost, design protocols so that the last *sender*
+    /// of data in the protocol calls [`Connection::closed`], and `await`s until
+    /// it receives a "close" message from the *receiver*. Once the *receiver*
+    /// gets the last data in the protocol, it should call [`Connection::close`]
+    /// to inform the *sender* that all data has been received.
     ///
     /// Be aware however that the underlying UDP sockets are only closed
     /// on [`Drop`], bearing in mind the [`Endpoint`] is only dropped once all the clones
@@ -1393,21 +1396,16 @@ impl Connection {
     ///
     /// # Gracefully closing a connection
     ///
-    /// Only the peer last receiving application data can be certain that all data is
-    /// delivered. The only reliable action it can then take is to close the connection,
-    /// potentially with a custom error code. The delivery of the final CONNECTION_CLOSE
-    /// frame is very likely if both endpoints stay online long enough, calling
-    /// [`Endpoint::close`] will wait to provide sufficient time. Otherwise, the remote peer
-    /// will time out the connection, provided that the idle timeout is not disabled.
-    ///
-    /// The sending side can not guarantee all stream data is delivered to the remote
-    /// application. It only knows the data is delivered to the QUIC stack of the remote
-    /// endpoint. Once the local side sends a CONNECTION_CLOSE frame in response to calling
-    /// [`close`] the remote endpoint may drop any data it received but is as yet
-    /// undelivered to the application, including data that was acknowledged as received to
-    /// the local endpoint.
-    ///
-    /// [`close`]: Connection::close
+    /// Only the peer last **receiving** application data can be certain that all data is
+    /// delivered.
+    ///
+    /// To communicate to the last **sender** of the application data that all the data was received, we recommend designing protocols that follow this pattern:
+    ///
+    /// 1) The **sender** sends the last data. It then calls [`Connection::closed`]. This will wait until it receives a CONNECTION_CLOSE frame from the other side.
+    /// 2) The **receiver** receives the last data. It then calls [`Connection::close`] and provides an error_code and/or reason.
+    /// 3) The **sender** checks that the error_code is the expected error code.
+    ///
+    /// If the `close`/`closed` dance is not done, or is interrupted at any point, the connection will eventually time out, provided that the idle timeout is not disabled.
     #[inline]
     pub fn close(&self, error_code: VarInt, reason: &[u8]) {
         self.inner.close(error_code, reason)

iroh/src/magicsock.rs

@@ -1785,12 +1785,19 @@ impl Handle {
     /// Only the first close does anything. Any later closes return nil.
     /// Polling the socket ([`AsyncUdpSocket::poll_recv`]) will return [`Poll::Pending`]
     /// indefinitely after this call.
+    ///
+    /// This will not wait for the [`quinn::Endpoint`] to drain connections.
+    ///
+    /// To ensure no data is lost, design protocols so that the last *sender*
+    /// of data in the protocol calls [`crate::endpoint::Connection::closed`], and `await`s until
+    /// it receives a "close" message from the *receiver*. Once the *receiver*
+    /// gets the last data in the protocol, it should call [`crate::endpoint::Connection::close`]
+    /// to inform the *sender* that all data has been received.
     #[instrument(skip_all, fields(me = %self.msock.me))]
     pub(crate) async fn close(&self) {
         trace!("magicsock closing...");
         // Initiate closing all connections, and refuse future connections.
         self.endpoint.close(0u16.into(), b"");
-        self.endpoint.wait_idle().await;
 
         if self.msock.is_closed() {
             return;