diff --git a/implementations/Crafter.Network-ClientQUIC.cpp b/implementations/Crafter.Network-ClientQUIC.cpp index 5962da7..b00f323 100644 --- a/implementations/Crafter.Network-ClientQUIC.cpp +++ b/implementations/Crafter.Network-ClientQUIC.cpp @@ -99,6 +99,21 @@ namespace { // only stops an msquic-side surprise from wedging the destructor outright. constexpr std::chrono::milliseconds streamDrainTimeout{ 5000 }; + // Bound on how long ~ClientQUIC then waits for the connection's shutdown + // to complete before it closes the handle. Closing earlier loses the + // CONNECTION_CLOSE frame: msquic sends it from a flush it queues while it + // processes ConnectionShutdown, a ConnectionClose that reaches the worker + // before that flush completes the shutdown silently (QuicConnTryClose's + // "silent close forced" path), and the flush is then skipped (FLUSH_SEND + // is ignored once ShutdownComplete is set). The peer learned of the close + // only at its idle timeout, two minutes by default; with no stream open + // the drain above returns at once, so that was the usual outcome. + // SHUTDOWN_COMPLETE comes about a round trip later, once the peer answers + // the close, or after msquic's closing period (three PTOs) when it does + // not. Either way the frame went out at the start, so the bound only + // stops a peer that is gone from holding the destructor that long. + constexpr std::chrono::milliseconds closeNotifyTimeout{ 1000 }; + // Encode an ALPN string into the wire format msquic expects: a length // byte followed by the ASCII characters. Lifetime of the returned buffer // matches the caller's storage in `out`. @@ -534,6 +549,7 @@ struct ClientQUIC::Impl { std::condition_variable cv; bool connected = false; bool closed = false; + bool shutdownComplete = false; // msquic delivered SHUTDOWN_COMPLETE QUIC_STATUS shutdownStatus = QUIC_STATUS_SUCCESS; std::function onStream; @@ -565,9 +581,10 @@ struct ClientQUIC::Impl { } // Shut the connection down and close it, waiting (bounded) for every - // stream on it to reach StreamClose first. No-op if someone else already - // claimed the handle. Never holds `mtx` across an msquic call: the - // connection callback takes it too. + // stream on it to reach StreamClose first, and then for the shutdown to + // complete, so the peer is told. No-op if someone else already claimed + // the handle. Never holds `mtx` across an msquic call: the connection + // callback takes it too. void CloseConnection() { HQUIC claimed = ClaimConnection(); if (!claimed) return; @@ -575,6 +592,13 @@ struct ClientQUIC::Impl { // Retire, which closes it and drops the count below. Runtime().api->ConnectionShutdown(claimed, QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, 0); streams->WaitForDrain(streamDrainTimeout); + { + // See closeNotifyTimeout. The SHUTDOWN_COMPLETE handler sets the + // flag before it looks at the handle, which we hold, so it leaves + // the close to us. + std::unique_lock lk(mtx); + cv.wait_for(lk, closeNotifyTimeout, [&] { return shutdownComplete; }); + } Runtime().api->ConnectionClose(claimed); } @@ -605,6 +629,7 @@ struct ClientQUIC::Impl { { std::lock_guard lk(self->mtx); self->closed = true; + self->shutdownComplete = true; } self->cv.notify_all(); // AppCloseInProgress means ~ClientQUIC is already inside diff --git a/interfaces/Crafter.Network-ClientQUIC.cppm b/interfaces/Crafter.Network-ClientQUIC.cppm index 015e6fb..7ccbde0 100644 --- a/interfaces/Crafter.Network-ClientQUIC.cppm +++ b/interfaces/Crafter.Network-ClientQUIC.cppm @@ -196,7 +196,10 @@ namespace Crafter { // // Lifetime: ~ClientQUIC shuts the connection down and closes it, waiting // (bounded — 5s) for every stream on it to finish closing first, since - // QUICStream::Stop only initiates a shutdown that completes asynchronously. + // QUICStream::Stop only initiates a shutdown that completes asynchronously, + // and then (bounded — 1s, about a round trip when the peer answers) for + // the shutdown to complete, so the peer is sent the close rather than + // left to time the connection out. // Streams outliving the connection are therefore safe to hold, though they // are dead once it goes; prefer destroying or moving them out first. //