fix(quic): let ~ClientQUIC's CONNECTION_CLOSE reach the peer
~ClientQUIC asked msquic to shut the connection down, waited for its streams to drain, and closed the handle. With no stream open the drain returns at once, so ConnectionClose followed ConnectionShutdown within microseconds. msquic sends the CONNECTION_CLOSE frame from a FLUSH_SEND operation it queues while it processes the shutdown; the close the destructor had queued meanwhile reached the worker first, forced the shutdown to complete silently (QuicConnTryClose: "silent close forced after we already started the close process"), and the flush, which is ignored once ShutdownComplete is set, sent nothing. The peer learned of the close only at its idle timeout, two minutes by default: a server whose clients each live for one request, such as a debug server driven by a CLI that connects once per command, kept every one of them that long. The destructor now also waits, after the drain, for the connection's SHUTDOWN_COMPLETE before closing the handle. That comes about one round trip later once the peer answers the close, or after msquic's closing period (three PTOs) when it does not; the wait is bounded at 1 s, since the frame went out at the start either way and a peer that is gone must not hold the destructor for the whole closing period. The handler sets a flag for it before it looks at the handle, which the destructor holds, so the close stays the destructor's. Measured with ShouldTellPeerOfCloseQUIC: before, the server was not told of the first of 20 client closes within 2 s (and, counted another way, 8 of 100 peer closes reached the server within 10 s in one run, 0 of 100 in the next); after, all 20 within a millisecond. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
76e927595d
commit
795f06178d
2 changed files with 32 additions and 4 deletions
|
|
@ -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<void(QUICStream)> 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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
//
|
||||
|
|
|
|||
Loading…
Reference in a new issue