diff --git a/implementations/Crafter.Network-ClientQUIC.cpp b/implementations/Crafter.Network-ClientQUIC.cpp index 5ba6e2e..a57d857 100644 --- a/implementations/Crafter.Network-ClientQUIC.cpp +++ b/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 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& notice) { + std::function 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 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 streams = std::make_shared(); + // OnClosed's callback and the state that decides whether it runs. + std::shared_ptr closeNotice = std::make_shared(); + 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 - // of the two actually does it. Every stream on the connection - // has already been shut down (and so closed) by the time - // msquic reports the connection complete. - if (ev->SHUTDOWN_COMPLETE.AppCloseInProgress == 0 - && self->ClaimConnection() != nullptr) { - Runtime().api->ConnectionClose(conn); + // owning ClientQUIC lives on. Claiming the handle settles which + // of the two actually does it. Every stream msquic knew of was + // aborted at the first close (QuicConnTryClose), so each has + // had its own SHUTDOWN_COMPLETE before this one. + if (ev->SHUTDOWN_COMPLETE.AppCloseInProgress != 0) return QUIC_STATUS_SUCCESS; + std::shared_ptr notice; + { + // 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)> cb) { impl->onDatagram = std::move(cb); } +void ClientQUIC::OnClosed(std::function callback) { + if (!impl) return; + const std::shared_ptr 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 ClientQUIC::RecieveDatagramSync() { std::unique_lock lk(impl->mtx); impl->cv.wait(lk, [&]{ return !impl->datagramQueue.empty() || impl->closed; }); diff --git a/interfaces/Crafter.Network-ClientQUIC.cppm b/interfaces/Crafter.Network-ClientQUIC.cppm index 7ccbde0..5c837c7 100644 --- a/interfaces/Crafter.Network-ClientQUIC.cppm +++ b/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 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 callback); #endif // Cleanly shut down the connection. diff --git a/project.cpp b/project.cpp index 7137192..a0f4f9d 100644 --- a/project.cpp +++ b/project.cpp @@ -197,6 +197,7 @@ extern "C" Configuration CrafterBuildProject(std::span 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 });