fix(iroh): Allow gracefully closing connections (#3170)

## Description

This adds a test case that checks that we can call `Connection::close`
and actually receive the close code on the other side, indicating we've
gracefully closed the connection.
Even though in the test we call `Endpoint::close` and wait for that,
with current iroh 0.32, we don't receive the close frame. The only way I
can see to make it work with iroh's 0.32 API, is to choose some time to
sleep before calling `Endpoint::close`.

I've also attached a revert of #3165 which fixes the test, as that makes
`Endpoint::close` wait for the close frames to be acknowledged again (by
calling `quinn::Endpoint::wait_idle`).

## Notes & open questions

There might be other ways to fix this, I welcome feedback on that. I do
believe this is the right way to fix it, though.

Given that the `Endpoint::wait_idle` call has been introduced and
removed *twice* already, we should finally make a choice on whether it
belongs in there or not, and then add sufficient code comments to make
sure whoever looks at it next is fully informed.

## 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.
- [x] Tests if relevant.
- [x] All breaking changes documented.

Author: matheus23

iroh/src/endpoint.rs

@@ -1023,17 +1023,32 @@ 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.
     ///
-    /// 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
-    /// are dropped.
+    /// 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 future
+    /// finishes.
+    ///
+    /// The maximum time-out that this future will wait for depends on QUIC transport
+    /// configurations of non-drained connections at the time of calling, and their current
+    /// estimates of round trip time. With default parameters and a conservative estimate
+    /// of round trip time, this call's future should take 3 seconds to resolve in cases of
+    /// bad connectivity or failed connections. In the usual case, this call's future should
+    /// return much more quickly.
+    ///
+    /// It is highly recommended you *do* wait for this close call to finish, if possible.
+    /// Not doing so will make connections that were still open while closing the endpoint
+    /// time out on the remote end. Thus remote ends will assume connections to have failed
+    /// even if all application data was transmitted successfully.
+    ///
+    /// Note: Someone used to closing TCP sockets might wonder why it is necessary to wait
+    /// for timeouts when closing QUIC endpoints, while they don't have to do this for TCP
+    /// sockets. This is due to QUIC and its acknowledgments being implemented in user-land,
+    /// while TCP sockets usually get closed and drained by the operating system in the
+    /// kernel during the "Time-Wait" period of the TCP socket.
+    ///
+    /// Be aware however that the underlying UDP sockets are only closed once all clones of
+    /// the the respective [`Endpoint`] are dropped.
     pub async fn close(&self) {
         if self.is_closed() {
             return;
@@ -1542,16 +1557,21 @@ impl Connection {
     ///
     /// # Gracefully closing a connection
     ///
-    /// 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.
+    /// 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
     #[inline]
     pub fn close(&self, error_code: VarInt, reason: &[u8]) {
         self.inner.close(error_code, reason)
@@ -2513,4 +2533,43 @@ mod tests {
 
         Ok(())
     }
+
+    #[tokio::test]
+    #[traced_test]
+    async fn graceful_close() -> testresult::TestResult {
+        let client = Endpoint::builder().bind().await?;
+        let server = Endpoint::builder()
+            .alpns(vec![TEST_ALPN.to_vec()])
+            .bind()
+            .await?;
+        let server_addr = server.node_addr().await?;
+        let server_task = tokio::spawn(async move {
+            let incoming = server.accept().await.unwrap();
+            let conn = incoming.await?;
+            let (mut send, mut recv) = conn.accept_bi().await?;
+            let msg = recv.read_to_end(1_000).await?;
+            send.write_all(&msg).await?;
+            send.finish()?;
+            let close_reason = conn.closed().await;
+            testresult::TestResult::Ok(close_reason)
+        });
+
+        let conn = client.connect(server_addr, TEST_ALPN).await?;
+        let (mut send, mut recv) = conn.open_bi().await?;
+        send.write_all(b"Hello, world!").await?;
+        send.finish()?;
+        recv.read_to_end(1_000).await?;
+        conn.close(42u32.into(), b"thanks, bye!");
+        client.close().await;
+
+        let close_err = server_task.await??;
+        let ConnectionError::ApplicationClosed(app_close) = close_err else {
+            panic!("Unexpected close reason: {close_err:?}");
+        };
+
+        assert_eq!(app_close.error_code, 42u32.into());
+        assert_eq!(app_close.reason.as_ref(), b"thanks, bye!");
+
+        Ok(())
+    }
 }

iroh/src/magicsock.rs

@@ -1809,20 +1809,29 @@ 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"");
 
+        // In the history of this code, this call had been
+        // - removed: https://github.com/n0-computer/iroh/pull/1753
+        // - then added back in: https://github.com/n0-computer/iroh/pull/2227/files#diff-ba27e40e2986a3919b20f6b412ad4fe63154af648610ea5d9ed0b5d5b0e2d780R573
+        // - then removed again: https://github.com/n0-computer/iroh/pull/3165
+        // and finally added back in together with this comment.
+        // So before removing this call, please consider carefully.
+        // Among other things, this call tries its best to make sure that any queued close frames
+        // (e.g. via the call to `endpoint.close(...)` above), are flushed out to the sockets
+        // *and acknowledged* (or time out with the "probe timeout" of usually 3 seconds).
+        // This allows the other endpoints for these connections to be notified to release
+        // their resources, or - depending on the protocol - that all data was received.
+        // With the current quinn API, this is the only way to ensure protocol code can use
+        // connection close codes, and close the endpoint properly.
+        // If this call is skipped, then connections that protocols close just shortly before the
+        // call to `Endpoint::close` will in most cases cause connection time-outs on remote ends.
+        self.endpoint.wait_idle().await;
+
         if self.msock.is_closed() {
             return;
         }