feat(quic): ClientQUIC::OnClosed tells the owner the connection is over

A ListenerQUIC hands each accepted connection to its callback as a
heap-allocated ClientQUIC the callback owns, and nothing said when to
delete it: the SHUTDOWN_COMPLETE handler closed the msquic handle of a
connection the peer or the idle timeout ended, but the object stayed, so
a server either leaked its peers or deleted them blind, and one still
alive at exit holds msquic's registration open until its idle timeout.

OnClosed(callback) is told once when the connection has shut down for
any reason but this object's destruction: the peer closed it, the idle
timeout or a transport error ended it, or Stop() was called. It runs on
the msquic worker, in the SHUTDOWN_COMPLETE handler, after that handler
has claimed and closed the handle, so deleting the ClientQUIC from
inside it is safe: ~ClientQUIC then finds no handle to claim and waits
for nothing. Delivering it there, rather than as a pool task, keeps it
independent of a pool the host shares and possibly saturates; the cost
is the usual worker rule, which the interface spells out (no blocking,
no *Sync call, no deleting another ClientQUIC from inside it). A
callback set after the close, which happens when the peer goes before a
ListenerQUIC's connectCallback runs, fires as a pool task instead, never
from inside OnClosed, where the caller is still using the object.

Its state is a shared CloseNotice rather than part of the Impl, since
the handler fires it after giving up the handle, when ~ClientQUIC on
another thread no longer waits for the worker and may free the Impl.
The rules are std::stop_callback's: ~ClientQUIC marks the owner gone
first, after which the callback never starts; one already running on
another thread is waited for, so the owner may free what it uses as
soon as the destructor returns; one on the destructor's own thread, a
callback deleting its connection, is not. The handler takes the handle
and the notice under one lock, so the destructor cannot slip between.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

implementations/Crafter.Network-ClientQUIC.cpp

@@ -86,6 +86,67 @@ namespace {
         }
     };
 
+    // ClientQUIC::OnClosed's state, shared by the ClientQUIC and whoever
+    // fires the callback: the SHUTDOWN_COMPLETE handler on an msquic worker,
+    // or a pool task when OnClosed is called after the close. Reached
+    // through a shared_ptr because the handler fires it after closing the
+    // connection handle, and from then on ~ClientQUIC, finding no handle
+    // left to claim, no longer waits for the worker and may free the Impl
+    // under it.
+    //
+    // The rules are std::stop_callback's: once ~ClientQUIC has set
+    // `ownerGone` the callback never starts; one already running on another
+    // thread is waited for, so the owner may free what the callback uses as
+    // soon as the destructor returns; one running on the destructor's own
+    // thread, a callback deleting its own connection, cannot be and is not.
+    struct CloseNotice {
+        std::mutex mtx;
+        std::condition_variable idle;
+        std::function<void()> callback;
+        bool closed = false;      // shut down, and not by ~ClientQUIC
+        bool fired = false;
+        bool ownerGone = false;   // ~ClientQUIC has begun
+        bool running = false;
+        std::thread::id runningOn;
+
+        // Runs the callback if it is due: closed, set, not yet run, and the
+        // owner still there.
+        static void Fire(const std::shared_ptr<CloseNotice>& notice) {
+            std::function<void()> callback;
+            {
+                std::lock_guard lk(notice->mtx);
+                if (!notice->closed || notice->fired || notice->ownerGone || !notice->callback) return;
+                notice->fired = true;
+                notice->running = true;
+                notice->runningOn = std::this_thread::get_id();
+                callback.swap(notice->callback);
+            }
+            callback();
+            // Destroyed before `running` drops, so a destructor waiting for
+            // us is never racing what the callback captured.
+            callback = nullptr;
+            {
+                std::lock_guard lk(notice->mtx);
+                notice->running = false;
+            }
+            notice->idle.notify_all();
+        }
+
+        // ~ClientQUIC's half: from here no callback starts, and one running
+        // on another thread has returned.
+        void OwnerGone() {
+            std::function<void()> dropped;
+            {
+                std::unique_lock lk(mtx);
+                ownerGone = true;
+                if (running && runningOn != std::this_thread::get_id()) {
+                    idle.wait(lk, [&] { return !running; });
+                }
+                dropped.swap(callback);
+            }
+        }
+    };
+
     // Bound on how long ~ClientQUIC waits for the connection's streams to
     // finish closing. The decrements arrive on msquic worker threads, so an
     // unbounded wait would turn a dropped peer into a hung destructor.
@@ -589,6 +650,9 @@ struct ClientQUIC::Impl {
     // so the count survives a stream that outlives us.
     std::shared_ptr<StreamRegistry> streams = std::make_shared<StreamRegistry>();
 
+    // OnClosed's callback and the state that decides whether it runs.
+    std::shared_ptr<CloseNotice> closeNotice = std::make_shared<CloseNotice>();
+
     ClientQUIC* outer = nullptr;
 
     // Take the connection handle away from whoever else might close it.
@@ -658,14 +722,29 @@ struct ClientQUIC::Impl {
                 // AppCloseInProgress means ~ClientQUIC is already inside
                 // ConnectionClose; otherwise close the handle here so a
                 // connection dropped by the peer does not leak while the
-                // owning ClientQUIC lives on. ClaimConnection settles which
+                // owning ClientQUIC lives on. Claiming the handle settles which
-                // of the two actually does it. Every stream on the connection
+                // of the two actually does it. Every stream msquic knew of was
-                // has already been shut down (and so closed) by the time
+                // aborted at the first close (QuicConnTryClose), so each has
-                // msquic reports the connection complete.
+                // had its own SHUTDOWN_COMPLETE before this one.
-                if (ev->SHUTDOWN_COMPLETE.AppCloseInProgress == 0
+                if (ev->SHUTDOWN_COMPLETE.AppCloseInProgress != 0) return QUIC_STATUS_SUCCESS;
-                    && self->ClaimConnection() != nullptr) {
+                std::shared_ptr<CloseNotice> notice;
-                    Runtime().api->ConnectionClose(conn);
+                {
+                    // The claim and the notice together, so ~ClientQUIC
+                    // cannot slip between them.
+                    std::lock_guard lk(self->mtx);
+                    if (self->connection == nullptr) return QUIC_STATUS_SUCCESS;
+                    self->connection = nullptr;
+                    notice = self->closeNotice;
                 }
+                Runtime().api->ConnectionClose(conn);
+                // Nothing of `self` from here on: ~ClientQUIC, finding no
+                // handle to claim, does not wait for this worker, and the
+                // OnClosed callback may well be what deletes it.
+                {
+                    std::lock_guard lk(notice->mtx);
+                    notice->closed = true;
+                }
+                CloseNotice::Fire(notice);
                 return QUIC_STATUS_SUCCESS;
             }
             case QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED: {
@@ -842,6 +921,13 @@ ClientQUIC::ClientQUIC(ClientQUIC&& other) noexcept
 
 ClientQUIC::~ClientQUIC() {
     if (!impl) return;
+    // First of all: the OnClosed callback must not start once the owner has
+    // begun destroying the connection, and one running on another thread
+    // has to be done with it before it goes. A callback that is deleting
+    // this very object runs on this thread and is not waited for; it already
+    // closed the handle, so CloseConnection below finds nothing to claim and
+    // returns at once.
+    impl->closeNotice->OwnerGone();
     // Before the connection goes: streams the peer started that no OnStream
     // handler ever claimed. Destroying them here rather than with the rest of
     // `impl` keeps every stream teardown ahead of ConnectionClose, so the
@@ -968,6 +1054,24 @@ void ClientQUIC::OnDatagram(std::function<void(std::vector<char>)> cb) {
     impl->onDatagram = std::move(cb);
 }
 
+void ClientQUIC::OnClosed(std::function<void()> callback) {
+    if (!impl) return;
+    const std::shared_ptr<CloseNotice> notice = impl->closeNotice;
+    bool due = false;
+    {
+        std::lock_guard lk(notice->mtx);
+        if (notice->fired) return;
+        // The one replaced is destroyed outside the lock, with `callback`.
+        callback.swap(notice->callback);
+        due = notice->closed && notice->callback;
+    }
+    // Closed already, typically before a ListenerQUIC's connectCallback got
+    // to run: fire from the pool, never from in here, where the caller is
+    // still using the object the callback is likely to delete. Fire checks
+    // again, so a second OnClosed racing this one cannot fire it twice.
+    if (due) ThreadPool::Enqueue([notice] { CloseNotice::Fire(notice); });
+}
+
 std::vector<char> ClientQUIC::RecieveDatagramSync() {
     std::unique_lock lk(impl->mtx);
     impl->cv.wait(lk, [&]{ return !impl->datagramQueue.empty() || impl->closed; });

interfaces/Crafter.Network-ClientQUIC.cppm

@@ -270,6 +270,30 @@ namespace Crafter {
         // Block the caller until the next datagram arrives; returns it.
         // Throws QUICClosedException if the connection closes first.
         std::vector<char> RecieveDatagramSync();
+
+        // Told once when the connection has shut down for any reason other
+        // than this object being destroyed: the peer closed it, the idle
+        // timeout or a transport error ended it, or Stop() was called. This
+        // is how the owner of a connection a ListenerQUIC accepted learns
+        // that it is time to delete it.
+        //
+        //   - It runs on the connection's msquic worker thread, once the
+        //     connection's handle is closed; if the connection had already
+        //     closed when OnClosed was called, it runs as a pool task
+        //     instead, never inside OnClosed itself.
+        //   - Deleting this ClientQUIC from inside it is safe, and the usual
+        //     thing to do. Do nothing else that blocks: deleting a
+        //     *different* ClientQUIC, or any *Sync call, waits on msquic
+        //     workers, this one perhaps among them. It must not throw; an
+        //     exception leaving it ends the program.
+        //   - Once ~ClientQUIC has begun it never starts, and a run already
+        //     under way on another thread finishes before ~ClientQUIC goes
+        //     on, as with std::stop_callback (one on this thread, deleting
+        //     its own connection, is not waited for). So do not destroy the
+        //     ClientQUIC while holding a lock the callback takes.
+        //   - Calling OnClosed again replaces the callback, and an empty one
+        //     unregisters it; once it has fired, later calls do nothing.
+        void OnClosed(std::function<void()> callback);
 #endif
 
         // Cleanly shut down the connection.

project.cpp

@@ -197,6 +197,7 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> a
             cfg.AddTest("ShouldFallbackUnknownRoutes").Dependencies({ &cfg });
             cfg.AddTest("ShouldHonourQUICSettings").Dependencies({ &cfg });
             cfg.AddTest("ShouldNotDropEarlyStreams").Dependencies({ &cfg });
+            cfg.AddTest("ShouldNotifyClosedConnectionQUIC").Dependencies({ &cfg });
             cfg.AddTest("ShouldSend").Dependencies({ &cfg });
             cfg.AddTest("ShouldSendRecieveHTTP").Dependencies({ &cfg });
             cfg.AddTest("ShouldSendRecieveKeepaliveHTTP").Dependencies({ &cfg });