Catcrafts/Crafter.Network
Watch
2
0
Fork
You've already forked Crafter.Network
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(quic): close stream handles whose async StreamStart lost the teardown race

Browse source 
A client that sends on a stream while its connection is being torn down —
the shape any long-lived client with periodic acks or keepalives has —
would hang at exit inside MsQuicRegistrationClose, and occasionally
bugcheck on an msquic worker thread instead.

Root cause: StreamStart is asynchronous. It returns QUIC_STATUS_PENDING
and queues the start onto the connection, so it can still fail later, and
it fails with QUIC_STATUS_INVALID_STATE whenever the connection is shut
down before the queued start runs. Passed QUIC_STREAM_START_FLAG_NONE,
msquic leaves such a stream neither started nor shut down, so
SHUTDOWN_COMPLETE never arrives. The stream close is driven entirely off
that event, so the handle stays open: ~ClientQUIC's bounded drain expires,
the connection is closed with a stream msquic still considers open, and
the registration's rundown at exit() waits forever for the handle.

Instrumenting the handle lifecycle over the reproducer shows one leaked
locally-opened stream per failing run, correlating 1:1 with a
START_COMPLETE carrying 0x1 (INVALID_STATE).

Fixes:

  - OpenStream passes QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL, so a start
    that loses the race is followed by SHUTDOWN_COMPLETE like any other
    stream and its handle is closed.

  - QUICStream::Stop also aborts the receive direction. GRACEFUL closes
    only the send side and msquic rejects it combined with any other flag,
    so a bidirectional stream whose peer keeps its send side open never
    reached SHUTDOWN_COMPLETE either.

  - The msquic stream handle is now refcounted, held by the callback plus
    every app thread inside an msquic call on it, and closed by whoever
    lets go last. StreamSend/StreamShutdown/GetParam could previously run
    against a handle a worker thread had just closed — a use-after-free
    msquic reports as a bugcheck. A refcount rather than an exclusion lock
    because GetParam blocks on the connection's worker, which must never
    be the thread waiting.

  - ~ClientQUIC drops undispatched peer streams before closing the
    connection rather than with the rest of Impl afterwards, so the drain
    covers them.

The reproducer (tests/ShouldSurviveConnectionChurn) grows a client-opened
bidirectional control stream with a thread sending across the teardown,
and no longer ends in std::_Exit — returning from main runs the static
MsQuicRuntime destructor, and its RegistrationClose is where a leaked
handle shows up. Before: 5 of 15 runs clean, 10 hung. After: 90 of 90
clean, with every opened handle observed closed and the drain never
expiring.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
catbot 2026-08-25 23:18:01 +00:00
commit ffc118f825
3 changed files with 231 additions and 76 deletions
Download patch file Download diff file Expand all files Collapse all files

199
implementations/Crafter.Network-ClientQUIC.cpp
View file

@ -52,7 +52,7 @@ namespace {
// Open-stream bookkeeping, shared by a connection and every stream on it.
// A stream counts as open from the moment msquic has a callback handler
// for it until FinalizeClose has run StreamClose. ~ClientQUIC drains this
// for it until its handle has been StreamClose'd. ~ClientQUIC drains this
// before closing the connection: QUICStream::Stop only *initiates* a
// shutdown whose async completion does the actual close, so without the
// wait a connection can be closed with streams still open on the msquic
@ -89,6 +89,14 @@ namespace {
// 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.
//
// Expiring is a bug, not a slow path: ConnectionShutdown makes msquic
// deliver SHUTDOWN_COMPLETE for every stream it knows about, OpenStream
// passes SHUTDOWN_ON_FAIL so a stream whose start lost the race to that
// shutdown gets one too, and QUICStream::Stop aborts the receive direction
// so a bidi stream does not sit waiting on a peer that will never close
// its send side. With those three the count always reaches zero; the bound
// only stops an msquic-side surprise from wedging the destructor outright.
constexpr std::chrono::milliseconds streamDrainTimeout{ 5000 };
// Encode an ALPN string into the wire format msquic expects: a length
@ -106,7 +114,6 @@ namespace {
// ---------------- QUICStream::Impl ----------------
struct QUICStream::Impl {
HQUIC handle = nullptr;
ClientQUIC* connection = nullptr;
std::mutex mtx;
@ -128,7 +135,7 @@ struct QUICStream::Impl {
// The owning connection's open-stream count. Incremented where the msquic
// handler is installed (QUICStream's handle constructor / OpenStream),
// decremented by FinalizeClose.
// decremented once the handle has been closed.
std::shared_ptr<StreamRegistry> registry;
// The connection's registry. Defined below ClientQUIC::Impl (which owns
@ -136,19 +143,73 @@ struct QUICStream::Impl {
// inherits QUICStream's friendship with ClientQUIC.
static std::shared_ptr<StreamRegistry> RegistryOf(ClientQUIC* connection);
// Detach the msquic handler, close the stream, and release the callback's
// strong ref. Called once, from SHUTDOWN_COMPLETE (normal path) or the
// OpenStream error path — never racing another close of the same stream.
static void FinalizeClose(Impl* self, HQUIC stream) {
Runtime().api->SetCallbackHandler(stream, nullptr, nullptr);
Runtime().api->StreamClose(stream);
std::shared_ptr<StreamRegistry> registry = std::move(self->registry);
std::shared_ptr<Impl>* ref = self->selfRef;
self->selfRef = nullptr;
delete ref;
// Last: a destructor blocked in WaitForDrain may run ConnectionClose
// the moment this hits zero, and msquic wants StreamClose first.
if (registry) registry->Remove();
// ---- msquic handle ownership ----------------------------------------
// StreamClose is "equivalent to free": any other msquic call on the handle
// that overlaps it is a use-after-free, which msquic reports as a bugcheck
// on whichever thread trips over the wreckage. The handle therefore has a
// refcount, and whoever drops the last ref closes it. There are two kinds
// of holder:
//
// - The msquic callback holds exactly one ref for the stream's lifetime,
// released by Retire() once SHUTDOWN_COMPLETE has been delivered (or by
// the OpenStream error paths, where no callback will ever run).
// - Every app thread inside an msquic call on the handle holds one for
// the duration of that call (AcquireHandle / ReleaseHandle).
//
// Without the second kind, a thread sending on a stream races the worker
// thread that closes it when the connection is torn down underneath —
// the shape any client with periodic acks or keepalives has.
std::mutex handleMtx;
HQUIC handle = nullptr; // cleared when the last ref lets go
int handleRefs = 1; // starts with the callback's ref
bool retired = false; // the callback's ref has been released
// The handle, valid until the matching ReleaseHandle; nullptr once the
// stream is retired, in which case do not call ReleaseHandle.
HQUIC AcquireHandle() {
std::lock_guard lk(handleMtx);
if (retired || !handle) return nullptr;
++handleRefs;
return handle;
}
void ReleaseHandle() {
HQUIC toClose = nullptr;
{
std::lock_guard lk(handleMtx);
if (--handleRefs > 0) return;
toClose = handle;
handle = nullptr;
}
// Last ref: nobody else can be inside an msquic call on this handle,
// and AcquireHandle will hand it out no more.
if (toClose) Runtime().api->StreamClose(toClose);
std::shared_ptr<StreamRegistry> reg = std::move(registry);
std::shared_ptr<Impl>* ref = selfRef;
selfRef = nullptr;
delete ref; // may destroy *this — touch no members past here
// Last of all: a destructor blocked in WaitForDrain may run
// ConnectionClose the moment this hits zero, and msquic wants every
// StreamClose on the connection to have happened first.
if (reg) reg->Remove();
}
// Publish the msquic handle. Called once, while the stream is still local
// to its constructor and no callback can run on it.
void SetHandle(HQUIC h) {
std::lock_guard lk(handleMtx);
handle = h;
}
// Release the callback's ref. Idempotent, so the SHUTDOWN_COMPLETE handler
// and an OpenStream error path can both call it without racing.
void Retire() {
{
std::lock_guard lk(handleMtx);
if (retired) return;
retired = true;
}
ReleaseHandle();
}
static QUIC_STATUS QUIC_API Callback(HQUIC stream, void* ctx, QUIC_STREAM_EVENT* ev) {
@ -200,9 +261,12 @@ struct QUICStream::Impl {
std::lock_guard lk(self->mtx);
self->peerSendClosed = true;
self->shutdownComplete = true;
// No further SEND_COMPLETE can arrive, so release anyone
// waiting on one — they throw on shutdownComplete instead.
self->sendInFlight = false;
}
self->cv.notify_all();
FinalizeClose(self, stream);
self->Retire();
return QUIC_STATUS_SUCCESS;
}
default:
@ -216,7 +280,7 @@ QUICStream::QUICStream() = default;
QUICStream::QUICStream(HQUIC handle, ClientQUIC* connection)
: handle(handle), connection(connection), impl(std::make_shared<Impl>())
{
impl->handle = handle;
impl->SetHandle(handle); // handleRefs already accounts for the callback
impl->connection = connection;
impl->registry = Impl::RegistryOf(connection);
if (impl->registry) impl->registry->Add();
@ -253,29 +317,34 @@ QUICStream::~QUICStream() {
void QUICStream::Stop() {
if (!handle) return;
// Only INITIATE a graceful shutdown here; the async SHUTDOWN_COMPLETE
// callback performs the actual StreamClose + ref release. The Impl is a
// shared_ptr held by the callback's selfRef, so it safely outlives this
// wrapper until that callback runs. If SHUTDOWN_COMPLETE already fired,
// msquic has closed the stream and calling StreamShutdown again would trip
// a bugcheck — skip it.
bool alreadyClosed = false;
if (impl) {
std::lock_guard lk(impl->mtx);
alreadyClosed = impl->shutdownComplete;
}
if (!alreadyClosed) {
Runtime().api->StreamShutdown(handle, QUIC_STREAM_SHUTDOWN_FLAG_GRACEFUL, 0);
}
handle = nullptr;
if (impl) {
std::lock_guard lk(impl->mtx);
impl->handle = nullptr;
handle = nullptr; // the wrapper is done with the stream either way
if (!impl) return;
// Only INITIATE the shutdown here; the async SHUTDOWN_COMPLETE callback
// releases the handle, and the Impl is a shared_ptr held by the callback's
// selfRef, so it safely outlives this wrapper until that callback runs.
// A null handle means SHUTDOWN_COMPLETE already fired and msquic has closed
// the stream — shutting it down again would be a use-after-free.
HQUIC live = impl->AcquireHandle();
if (!live) return;
// GRACEFUL closes our send direction only, and msquic rejects it combined
// with any other flag, so the receive direction needs a call of its own.
// Without it a bidirectional stream whose peer keeps its send side open
// never reaches SHUTDOWN_COMPLETE, so its handle is never closed and the
// connection can never finish tearing down.
Runtime().api->StreamShutdown(live, QUIC_STREAM_SHUTDOWN_FLAG_GRACEFUL, 0);
if (canReceive) {
Runtime().api->StreamShutdown(live, QUIC_STREAM_SHUTDOWN_FLAG_ABORT_RECEIVE, 0);
}
impl->ReleaseHandle();
}
void QUICStream::SendSync(const void* buffer, std::uint32_t size, bool finish) {
if (!handle || !canSend) throw QUICClosedException();
// Hold the handle across StreamSend: without this a connection torn down
// on another thread can close the stream between the check above and the
// call below, and msquic bugchecks on the freed handle.
HQUIC live = impl->AcquireHandle();
if (!live) throw QUICClosedException();
auto* copy = new char[size];
std::memcpy(copy, buffer, size);
QUIC_BUFFER quicBuf{};
@ -286,8 +355,15 @@ void QUICStream::SendSync(const void* buffer, std::uint32_t size, bool finish) {
impl->sendInFlight = true;
}
QUIC_SEND_FLAGS flags = finish ? QUIC_SEND_FLAG_FIN : QUIC_SEND_FLAG_NONE;
QUIC_STATUS s = Runtime().api->StreamSend(handle, &quicBuf, 1, flags, copy);
QUIC_STATUS s = Runtime().api->StreamSend(live, &quicBuf, 1, flags, copy);
impl->ReleaseHandle();
if (QUIC_FAILED(s)) {
{
// No SEND_COMPLETE will arrive for a send msquic never took.
std::lock_guard lk(impl->mtx);
impl->sendInFlight = false;
}
impl->cv.notify_all();
delete[] copy;
throw QUICException(std::format("StreamSend failed: 0x{:x}", static_cast<unsigned>(s)));
}
@ -380,9 +456,15 @@ void QUICStream::PrependReceived(std::vector<char> bytes) {
std::uint64_t QUICStream::GetStreamId() const {
if (!handle) throw QUICException("GetStreamId: stream is not open");
// GetParam blocks on the connection's msquic worker, so it must not be the
// worker that waits for us — hence a refcount rather than an exclusion
// lock: SHUTDOWN_COMPLETE just drops its ref and returns.
HQUIC live = impl->AcquireHandle();
if (!live) throw QUICException("GetStreamId: stream is not open");
QUIC_UINT62 id = 0;
std::uint32_t size = sizeof(id);
QUIC_STATUS s = Runtime().api->GetParam(handle, QUIC_PARAM_STREAM_ID, &size, &id);
QUIC_STATUS s = Runtime().api->GetParam(live, QUIC_PARAM_STREAM_ID, &size, &id);
impl->ReleaseHandle();
if (QUIC_FAILED(s)) {
throw QUICException(std::format("GetParam(QUIC_PARAM_STREAM_ID) failed: 0x{:x}",
static_cast<unsigned>(s)));
@ -438,7 +520,7 @@ struct ClientQUIC::Impl {
HQUIC claimed = ClaimConnection();
if (!claimed) return;
// Aborts any stream still open; each one's SHUTDOWN_COMPLETE runs
// FinalizeClose, which closes it and drops the count below.
// Retire, which closes it and drops the count below.
Runtime().api->ConnectionShutdown(claimed, QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, 0);
streams->WaitForDrain(streamDrainTimeout);
Runtime().api->ConnectionClose(claimed);
@ -478,8 +560,8 @@ struct ClientQUIC::Impl {
// 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 FinalizeClose)
// by the time msquic reports the connection complete.
// 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);
@ -660,6 +742,17 @@ ClientQUIC::ClientQUIC(ClientQUIC&& other) noexcept
ClientQUIC::~ClientQUIC() {
if (!impl) return;
// 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
// drain below actually covers them.
{
std::deque<QUICStream> backlog;
{
std::lock_guard lk(impl->mtx);
std::swap(backlog, impl->pendingStreams);
}
}
impl->CloseConnection();
if (impl->configuration && impl->ownsConfiguration) {
Runtime().api->ConfigurationClose(impl->configuration);
@ -699,26 +792,36 @@ QUICStream ClientQUIC::OpenStream(bool unidirectional) {
if (QUIC_FAILED(s)) {
delete stream.impl->selfRef;
stream.impl->selfRef = nullptr;
// No msquic stream exists, so FinalizeClose is not the right undo.
// No msquic stream exists, so there is no handle to release.
std::shared_ptr<StreamRegistry> registry = std::move(stream.impl->registry);
registry->Remove();
throw QUICException(std::format("StreamOpen failed: 0x{:x}", static_cast<unsigned>(s)));
}
stream.handle = streamHandle;
stream.connection = this;
stream.impl->handle = streamHandle;
stream.impl->SetHandle(streamHandle);
if (unidirectional) {
// We initiated the unidi stream: we send, peer reads.
stream.canSend = true;
stream.canReceive = false;
}
s = Runtime().api->StreamStart(streamHandle, QUIC_STREAM_START_FLAG_NONE);
// SHUTDOWN_ON_FAIL, not NONE. StreamStart is asynchronous — it returns
// QUIC_STATUS_PENDING and queues the start onto the connection — so it can
// still fail later, and it does exactly that (QUIC_STATUS_INVALID_STATE)
// whenever the connection is shut down before the queued start runs. That
// is an everyday race for a client that sends periodically: opening a
// stream just as the connection goes away. Without this flag msquic leaves
// such a stream neither started nor shut down, so SHUTDOWN_COMPLETE never
// arrives, the handle is never closed, and the registration's rundown at
// exit() blocks forever waiting for it. With it, the failed start is
// followed by SHUTDOWN_COMPLETE like any other stream.
s = Runtime().api->StreamStart(streamHandle, QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL);
if (QUIC_FAILED(s)) {
// Handler is registered; detach + close so no callback fires on the
// failed stream, and clear the wrapper handle so ~QUICStream is a no-op.
QUICStream::Impl::FinalizeClose(stream.impl.get(), streamHandle);
// Synchronous failure: the stream never started, so no callback will
// ever run. Release the callback's ref by hand, which closes the
// handle, and clear the wrapper's so ~QUICStream is a no-op.
stream.handle = nullptr;
stream.impl->handle = nullptr;
stream.impl->Retire();
throw QUICException(std::format("StreamStart failed: 0x{:x}", static_cast<unsigned>(s)));
}
return stream;