diff --git a/cpp/implementations/plain/plain_logos_object.cpp b/cpp/implementations/plain/plain_logos_object.cpp index e2edfaa..4d9eef4 100644 --- a/cpp/implementations/plain/plain_logos_object.cpp +++ b/cpp/implementations/plain/plain_logos_object.cpp @@ -9,6 +9,14 @@ #include #include +#include +#include +#include +#include +#include +#include +#include + #include #include #include @@ -16,82 +24,16 @@ #include #include #include +#include namespace logos::plain { namespace { -// How long a waiter sleeps before it looks at the stop flag again. -// -// A std::future wait cannot be interrupted, so the only way to make one -// abandonable is to wait in slices against the same overall deadline and check -// the flag between them. That slice IS the teardown-latency bound: releasing a -// handle with a call in flight costs at most one of these, instead of whatever -// is left of the call's timeout (20s on the protocol default, logos_mode.h). -// -// 25ms is chosen off both ends of the trade: -// * latency — a module unloading mid-call should feel instantaneous. 25ms is -// under two 60Hz frames and well below the ~100ms at which a stall becomes -// perceptible, so even a shutdown releasing handles back to back stays -// invisible. -// * cost — one timed wakeup per slice per IN-FLIGHT call: 40/s, i.e. 800 -// spread over a full 20s default timeout, and paid only while a call is -// actually outstanding. That is far below the wakeup rate of the Qt event -// loop these waiters already sit beside. -// Below ~5ms the extra wakeups buy latency nobody can perceive; at 100-250ms -// the teardown hitch starts to show. -constexpr std::chrono::milliseconds kWaitSlice(25); - -enum class WaitOutcome { Ready, TimedOut, Cancelled }; - -// ── the one rule both waits follow ────────────────────────────────────────── -// -// AN ANSWER ALREADY IN HAND BEATS A CONCURRENT STOP. The stop only decides what -// happens when there is nothing to hand over. -// -// The two sites used to resolve this in opposite directions — this one tested -// the flag before polling, so an already-ready future was still reported as -// transport_error, while awaitCompletion deliberately preferred a completion -// that had landed. Both were commented as deliberate, and they cannot both be -// right, so: the callback fires either way (postToQtEventLoop copies everything -// it delivers, precisely so a released handle costs it nothing), which means the -// only thing a stop can change is what the callback SAYS. Reporting -// transport_error while the true answer sits in the future is a failure that did -// not happen, and that code is not inert — callers re-acquire, retry and log on -// it. Preferring the answer is also free: it is already there, so nothing waits -// for it. The teardown-latency bound is untouched, because the flag is still -// checked before every sleep, and a stop with no answer in hand still wins -// immediately. -// -// The interruptible form of `fut.wait_for(milliseconds(timeoutMs))`. -// -// The overall deadline is computed once, so slicing does not stretch the -// timeout the caller asked for: the last slice ends exactly on it. -WaitOutcome waitForResult(std::future& fut, int timeoutMs, - const std::atomic& stopping) -{ - using clock = std::chrono::steady_clock; - const auto deadline = clock::now() + std::chrono::milliseconds(timeoutMs); - for (;;) { - // Poll FIRST, with a zero wait: an answer in hand beats both a - // concurrent stop and the deadline. This is also what makes a - // non-positive timeout behave as the single unsliced wait_for did — - // one poll, then give up — and what reports a future that went ready - // during the last slice. - if (fut.wait_for(clock::duration::zero()) == std::future_status::ready) - return WaitOutcome::Ready; - // Checked before sleeping, so a stop that already happened costs - // nothing, and after every slice, so one that arrives mid-wait costs at - // most kWaitSlice. - if (stopping.load(std::memory_order_acquire)) - return WaitOutcome::Cancelled; - - const auto remaining = deadline - clock::now(); - if (remaining <= clock::duration::zero()) - return WaitOutcome::TimedOut; - fut.wait_for(std::min(kWaitSlice, remaining)); - } -} +// The default the deferred half of a call falls back to when the caller gave a +// non-positive timeout — the value awaitCompletion has always used, kept so the +// async path and the sync path give up at the same moment. +constexpr int kDeferredFallbackMs = 30000; // The honest code for "the object was released while your call was in flight". // @@ -115,8 +57,320 @@ logos::CallError callErrorReleased(const std::string& objectName, "was released while the call was in flight"); } +// ----------------------------------------------------------------------------- +// DeadlineService — the clock the per-call deadlines hang off. ONE thread for +// the whole process, and deliberately NOT the one the connections run on. +// +// WHY IT IS SEPARATE, which is the single most important decision in this file. +// Folding the per-call waiter thread away means the deadline has to live +// somewhere else, and the obvious somewhere — the connection's own strand, on +// IoContextPool::shared() — makes every deadline in the process hostage to that +// one io thread. It is not a theoretical hostage: this transport delivers user +// onEvent callbacks INLINE on the io thread (rpc_connection.h dispatchIncoming), +// and an event handler that calls another module is ordinary module code. A +// handler making a 2000ms synchronous call while a 200ms deadline is outstanding +// on a COMPLETELY DIFFERENT connection made that deadline fire at 2003ms; +// measured, and the reason this class exists. With the ambient timeouts in this +// stack — 5s in getMethods, 30s in the deferred fallback — a 300ms deadline +// becomes multi-second, and a handler that blocks forever means the deadline +// never fires at all. The whole point of a timeout is that it is the thing that +// still works when everything else is stuck. +// +// The three alternatives, and why not: +// +// * A SECOND io thread in IoContextPool. Does not fix it — user handlers are +// unbounded, so N simultaneously-blocked handlers need N+1 threads, and the +// count is not knowable. It would also quietly break every "serialized by +// there being one thread" assumption in the transport, which is a far larger +// blast radius than this class. +// * MOVING INLINE EVENT DELIVERY OFF THE STRAND (post user callbacks to the +// Qt loop). Correct direction, much bigger change: it alters event ordering +// and re-entrancy for every existing consumer of this transport, and it does +// not help a deadline while the Qt loop itself is blocked. +// * A Qt TIMER on the Qt event loop. Strictly worse than either: a +// synchronous callMethod issued from the Qt thread — the most ordinary thing +// a module does — blocks that loop for the whole call, so the deadline would +// be hostage to exactly the calls it is supposed to bound. +// +// So: one dedicated thread, process-wide, that does nothing but arm, cancel and +// fire timers. It restores the independence the per-call waiter threads had, at +// one thread instead of one per pending call, which is the entire point of the +// fold. Nothing else may ever be posted here; user code reaches the Qt loop via +// postToQtEventLoop, and AsyncCall::deliver() is a flag, two map erases and a +// post. +// ----------------------------------------------------------------------------- +class DeadlineService { +public: + static DeadlineService& shared() + { + // Lazy, like IoContextPool::shared(): a process that never makes an + // async plain call never starts this thread. + static DeadlineService svc; + return svc; + } + + boost::asio::io_context& context() { return m_ioc; } + + DeadlineService(const DeadlineService&) = delete; + DeadlineService& operator=(const DeadlineService&) = delete; + +private: + DeadlineService() + : m_guard(boost::asio::make_work_guard(m_ioc)) + , m_thread([this] { m_ioc.run(); }) + {} + + ~DeadlineService() + { + m_guard.reset(); + m_ioc.stop(); + if (m_thread.joinable()) + m_thread.join(); + } + + boost::asio::io_context m_ioc; + boost::asio::executor_work_guard m_guard; + std::thread m_thread; +}; + +// The one place the choice above is made. Every per-call deadline in the process +// is armed on this context and nothing else is ever posted to it. +// +// The rejected design is one token different — IoContextPool::shared() +// .ioContext(), the connections' own thread — which is what makes the two tests +// in test_iofold.cpp that measure deadline accuracy under io-thread load worth +// having, and how they were validated. See that file for the numbers. +boost::asio::io_context& deadlineContext() +{ + return DeadlineService::shared().context(); +} + +// Hand `callback(result)` over to the Qt event loop so PlainLogosObject's +// async path matches LogosObject's interface contract: callbacks are +// always delivered on a subsequent event-loop iteration, on the Qt +// thread, never synchronously and never racing with QObjects/UI code. +// +// Using QCoreApplication::instance() as the anchor means the queued +// invocation lands on whichever thread runs the Qt event loop in this +// process, regardless of which worker thread completed the call. +// If the application has shut down (instance() is null), we drop the +// callback rather than invoke it from an arbitrary thread. +// +// Deliberately a FREE function taking everything BY VALUE, and deliberately not +// a member: the queued lambda runs on a later event-loop iteration, which for a +// call cancelled by teardown is after the PlainLogosObject is already gone. +// Nothing it touches may belong to the object — which is why AsyncCall copies +// objectName/method up front instead of reading m_objectName from inside here. +// Do not give this a `this`. +// +// THE FOLD MADE THIS LOAD-BEARING TWICE OVER. It was already the reason a +// delivery could outlive the handle. It is now also the answer to the +// re-entrancy hazard: every one of the four places that can complete a call — +// the Asio read handler on the connection's strand, fail()'s sweep on an +// arbitrary thread, the deadline handler on the timer thread, and teardown on +// the caller's thread — routes its delivery through here, so NO user callback +// ever runs on an Asio handler stack. That is the class of bug that produced the +// deferred-multi SIGSEGV on the QtRO twin, whose fix (remote_transport.cpp) is +// the same move by a different vehicle: QTimer::singleShot(0). +void postToQtEventLoop(PlainLogosObject::AsyncResultErrorCallback callback, + QVariant result, logos::CallError err) +{ + QCoreApplication* app = QCoreApplication::instance(); + if (!app) return; + QMetaObject::invokeMethod(app, + [callback = std::move(callback), result = std::move(result), + err = std::move(err)]() mutable { + callback(result, err); + }, + Qt::QueuedConnection); +} + } // anonymous namespace +// ----------------------------------------------------------------------------- +// AsyncCall — one in-flight asynchronous call. THIS IS WHAT REPLACED THE THREAD. +// +// The old design gave every pending RPC an OS thread whose only job was to be +// blockable: std::future cannot be waited on with a deadline AND a cancel, so +// the waiter polled it in 25ms slices, then parked on a condition variable for +// the deferred half, then delivered. Three costs came with that — a thread per +// pending call, a 25ms floor on teardown, and a registry-plus-reaping protocol +// to stop finished threads accumulating (a thread cannot join itself). +// +// Here the call is a piece of STATE that three events race to finish: +// +// * the reply, delivered by RpcConnection as a handler (its strand, or any +// thread via fail(), or inline when the connection is already stopped); +// * the deadline, an asio::steady_timer on the DeadlineService's own thread; +// * cancellation, from teardown on an arbitrary thread. +// +// EXACTLY ONCE is the `claim()` CAS below, and nothing else. That is a real +// change of mechanism and the thing most worth distrusting: the old guarantee +// was structural (one thread, one function, four returns, and a join proving it +// had finished), whereas three independent callers can all arrive here. The CAS +// is what makes the first one win and the other two no-ops, on every path. +// +// LIFETIME is ownership, not a barrier. Each of those three holds a shared_ptr +// to this object; the state it needs on the HANDLE is reached through a weak_ptr +// to CallState, and the connection through a weak_ptr too. Nothing here +// dereferences the PlainLogosObject, so release()'s `delete this` is none of its +// business and teardown has nothing to wait for. +// ----------------------------------------------------------------------------- +struct AsyncCall : std::enable_shared_from_this { + using clock = std::chrono::steady_clock; + + AsyncCall(std::weak_ptr st, + std::weak_ptr cn, + std::uint64_t callNumber, + std::string obj, std::string meth, int timeout, + PlainLogosObject::AsyncResultErrorCallback cb) + // A strand of its own over the shared deadline thread. With one thread + // in that service the strand is redundant today; it is here so that + // "every touch of this timer is serialized" stays a property of the + // code rather than of the thread count, because asio timers are + // "Shared objects: Unsafe" and a second service thread would otherwise + // turn a re-arm racing its own handler into undefined behaviour. + : timer(boost::asio::make_strand(deadlineContext())) + , state(std::move(st)) + , conn(std::move(cn)) + , id(callNumber) + , objectName(std::move(obj)) + , method(std::move(meth)) + , timeoutMs(timeout) + , callback(std::move(cb)) + {} + + boost::asio::steady_timer timer; + std::weak_ptr state; + // Only ever used to withdraw this call's registration from the connection's + // pending map — see cancelPending(). weak, because the connection outlives + // the handle but not necessarily this call's last handler. + std::weak_ptr conn; + const std::uint64_t id; + const std::string objectName; + const std::string method; + const int timeoutMs; + + // Set under CallState::mu when a "multi" provider defers, read under it in + // deliver() — the one field two threads can reach. + QString callId; + + std::mutex cbMu; + PlainLogosObject::AsyncResultErrorCallback callback; + std::atomic delivered{false}; + + // ── the exactly-once gate ──────────────────────────────────────────────── + // + // Three independent callers race for the right to resolve a call — the + // reply handler, the deadline and teardown — and exactly one may reach the + // user's callback. Two halves, INDEPENDENTLY SUFFICIENT, which is worth + // recording because it means neither is redundant: the CAS is the one that + // also skips the registry erase, the pending withdrawal and the timer + // cancel, and the swap is what makes the callback itself unrepeatable. + // + // This replaced a structural guarantee — one waiter thread, one function + // body, and a join proving it had finished — so it is the guarantee in this + // file most worth distrusting, and the two tests that actually detect its + // absence are named in tests/protocol/CMakeLists.txt. The per-path + // exactly-once assertions are NOT among them: a call resolved once calls + // deliver() once whatever guards it. + bool claim() + { + bool expected = false; + return delivered.compare_exchange_strong(expected, true, + std::memory_order_acq_rel); + } + + PlainLogosObject::AsyncResultErrorCallback takeCallback() + { + std::lock_guard g(cbMu); + PlainLogosObject::AsyncResultErrorCallback cb; + cb.swap(callback); + return cb; + } + + // Idempotent by construction: every later caller returns without touching + // the callback, the connection or the timer. + void deliver(QVariant value, logos::CallError err) + { + // LEAVE THE HANDLE'S REGISTRIES FIRST — before the exactly-once gate, + // and unconditionally, which is not where it reads most naturally. + // + // A duplicate deliver() has something to clean up. A provider can + // answer the pending sentinel AFTER this call's deadline has already + // passed: the timer resolves the call, and only then does the reply + // handler arrive, see a sentinel, and file this AsyncCall under + // CallState::deferred. Behind the gate, that entry would never be taken + // out again — one leaked map entry per slow-sentinel call, for the life + // of the handle, which is precisely the retention the fold exists to + // fix. (The reply handler also checks `delivered` before filing, so this + // only has to cover the instant between that check and the write; the + // re-armed deadline is what eventually runs this erase.) + // + // Erasing twice is free: ids come from RpcConnection::nextId() and are + // unique per call, so this can never take out somebody else's entry. + if (auto st = state.lock()) { + std::lock_guard g(st->mu); + st->inflight.erase(id); + if (!callId.isEmpty()) st->deferred.erase(callId); + } + + if (!claim()) return; + + PlainLogosObject::AsyncResultErrorCallback cb = takeCallback(); + + // AND LEAVE THE CONNECTION'S. m_pendingCalls is erased by exactly two + // events on its own — a decoded reply with this id, and fail()'s + // teardown sweep — so a call resolved by its DEADLINE, or by teardown of + // the handle rather than of the connection, used to leave its + // registration there for the whole life of the connection, which + // outlives every handle it hands out. That was true of the promise this + // replaced too; it is closed here rather than inherited. When the reply + // IS what got us here, dispatchIncoming has already erased it and this + // is a lookup that finds nothing. + if (auto c = conn.lock()) c->cancelPending(id); + + cancelTimer(); + // Last, and never with a lock held: the delivery hop. + if (cb) postToQtEventLoop(std::move(cb), std::move(value), std::move(err)); + } + + // Callable from ANY thread: the arm is POSTED onto the timer's own strand, + // so the timer object itself is only ever touched from the deadline thread. + // + // `when` is an ABSOLUTE deadline computed by the caller, not a duration, so + // the post hop cannot stretch it — the timer fires when the caller said it + // would even if the deadline thread is momentarily busy. `reportMs` is only + // what the timeout REPORTS; it diverges from the wall time for the deferred + // half of a non-positive-timeout call, which falls back to 30s the way + // awaitCompletion always has. + void armTimer(clock::time_point when, int reportMs) + { + auto self = shared_from_this(); + boost::asio::post(timer.get_executor(), [self, when, reportMs] { + self->timer.expires_at(when); + self->timer.async_wait([self, reportMs](const boost::system::error_code& ec) { + if (ec == boost::asio::error::operation_aborted) return; + self->deliver(QVariant(), + logos::callErrorTimeout(self->objectName, + self->method, reportMs)); + }); + }); + } + + // Also posted, for the same reason. Cancelling matters for retention rather + // than correctness (the CAS already makes a late timeout a no-op): without + // it, a call answered in 1ms with a 20s timeout would keep this object alive + // for the remaining 19.999s. + void cancelTimer() + { + auto self = shared_from_this(); + boost::asio::post(timer.get_executor(), [self] { + try { self->timer.cancel(); } catch (...) {} + }); + } +}; + PlainLogosObject::PlainLogosObject(std::string objectName, std::shared_ptr conn) : m_objectName(std::move(objectName)) @@ -127,119 +381,56 @@ PlainLogosObject::PlainLogosObject(std::string objectName, PlainLogosObject::~PlainLogosObject() { disconnectEvents(); - stopAndJoinWaiters(); + stopAndCancelCalls(); } -void PlainLogosObject::stopWaiters() +void PlainLogosObject::stopAndCancelCalls() { + std::vector> outstanding; { - // Published under the rendezvous mutex — the one awaitCompletion - // evaluates its predicate under — so a waiter cannot read `false`, - // decide to sleep, and only then miss the notify_all below. The sliced - // future wait reads the same flag lock-free, which is why it is an - // atomic rather than a plain bool guarded by this mutex. - std::lock_guard g(m_completion->mu); - m_stopping.store(true, std::memory_order_release); + // The flag is published under the same mutex awaitCompletion evaluates + // its predicate under, so the parked synchronous caller cannot read + // `false`, decide to sleep, and only then miss the notify_all below. + std::lock_guard g(m_state->mu); + m_state->stopping.store(true, std::memory_order_release); + outstanding.reserve(m_state->inflight.size()); + for (auto& entry : m_state->inflight) + outstanding.push_back(entry.second); + m_state->inflight.clear(); + m_state->deferred.clear(); + // Buffered completions nobody can claim any more. They are dropped here + // rather than left to the state block's own destruction so that a + // handler still holding a share of it does not keep them alive. + m_state->completions.clear(); } - m_completion->cv.notify_all(); -} + m_state->cv.notify_all(); -void PlainLogosObject::publishFinishedWaiter(std::uint64_t id) -{ - std::lock_guard g(m_waiterMu); - m_finishedWaiters.push_back(id); -} + // Cancelled OUTSIDE the lock, because deliver() takes it to erase its own + // registry entries. (It will find nothing — they were just cleared — which + // is fine and is why this cannot deadlock either way.) + // + // A cancelled call still DELIVERS, exactly once. Returning silently would + // honour the "stop fast" half and break the half that matters more: + // callMethodAsyncWithError (and lp_invoke_async above it) promise the + // callback fires exactly once, so a dropped one turns a bounded stall into + // an unbounded hang in every caller that awaits it. + for (auto& call : outstanding) + call->deliver(QVariant(), callErrorReleased(m_objectName, call->method)); -void PlainLogosObject::reapFinishedWaiters() -{ - // Only ids a waiter published are taken, and publishing is that waiter's - // last act — so everything moved into `done` has already stopped touching - // this object, and joining it is effectively instant. - std::vector done; - { - std::lock_guard g(m_waiterMu); - std::vector keep; - for (const std::uint64_t id : m_finishedWaiters) { - const auto it = m_waiters.find(id); - if (it == m_waiters.end()) - continue; // teardown already took this one - if (it->second.get_id() == std::this_thread::get_id()) { - // A waiter DOES run this now, on its way out — but always - // BEFORE it publishes, so its own id cannot be in the list it - // is walking, and this branch stays unreachable. It costs one - // comparison, and it turns the ordering slip that would make it - // reachable (publishing before reaping) into a leaked entry - // rather than a self-join, which throws out of the noexcept - // destructor doing the reaping and takes the process with it. - // Leave it registered; the next reaper, or teardown, collects it. - keep.push_back(id); - continue; - } - done.push_back(std::move(it->second)); - m_waiters.erase(it); - } - m_finishedWaiters.swap(keep); - } - // Joined with NO lock held. The deadlock this whole mechanism can - // introduce is a reaper that holds m_waiterMu while it joins a waiter which - // is itself blocked on m_waiterMu trying to publish. TWO INDEPENDENT - // PROPERTIES each prevent it, and either one alone would be enough: + // And that is the whole of teardown. NOTHING IS WAITED FOR: no thread to + // join, no io-thread barrier. The reply handler and the deadline handler for + // a cancelled call may still be queued; each holds its own shared_ptr to the + // AsyncCall, finds the gate already taken, and drops its share. None of them + // can reach this object, so it may be deleted the instant this returns. // - // * only PUBLISHED ids are joined, and publishing is a waiter's last - // access — so a thread this function joins can never be a thread that - // still wants m_waiterMu; - // * no join happens with a lock held, so even joining a thread that DID - // still want the mutex could not shut it out. - // - // Both are kept on purpose: the filter is a property of the logic here, - // which a refactor can lose without looking wrong, while "no join under a - // lock" is structural and tends to survive one. Be precise about what that - // costs in testing, though — the hammer in the regression suite only wedges - // when BOTH are gone. A variant that joins under the lock but keeps the - // published-only filter passes it, measured, in the usual few hundred ms. - // stopAndJoinWaiters() keeps the same discipline. - for (auto& t : done) { - if (t.joinable()) - t.join(); - } -} - -void PlainLogosObject::stopAndJoinWaiters() -{ - stopWaiters(); - - std::map waiters; - { - std::lock_guard g(m_waiterMu); - waiters.swap(m_waiters); - } - // Joined with NO lock held: a waiter on its way out still takes - // the rendezvous mutex (awaitCompletion) and then m_waiterMu (to publish), - // and m_waiterMu is also what a concurrent callMethodAsyncWithError needs - // in order to see the stop flag. - // - // Everything outstanding is joined by id-independent brute force, so this - // needs no cooperation from the reaper: a waiter that publishes while this - // loop runs simply leaves a stale id behind, and its thread is joined here - // anyway. - // - // A waiter that a concurrent reaper is in the middle of joining is NOT in - // this map, and that is still safe. The invariant is not "every waiter has - // been joined by the time this returns" but the thing that invariant was - // ever for: NO WAITER TOUCHES THIS OBJECT AFTER THIS RETURNS. An entry - // leaves m_waiters only once its thread has published, and publishing is - // that thread's last access — all it has left to do is unwind. - for (auto& entry : waiters) { - std::thread& t = entry.second; - if (t.joinable()) - t.join(); - } - // Cleared after the joins, so the stale ids just described go too. Nothing - // can be added afterwards: m_stopping is set, so no new waiter registers. - { - std::lock_guard g(m_waiterMu); - m_finishedWaiters.clear(); - } + // THE BARRIER THAT LOOKS RIGHT AND ISN'T: "post a no-op onto the connection + // strand and wait for it" would prove no handler is mid-flight, and it + // wedges the process — IoContextPool runs exactly ONE thread, this transport + // delivers user event callbacks inline on it, and release()-from-an-event- + // callback is shipped behaviour (remote_transport.cpp), so the caller can BE + // the only thread that could drain the barrier. Tried, deadlocked, + // discarded; ownership is what replaced the join, not a barrier. + // test_iofold.cpp pins the reentrant case with a watchdog. } QVariant PlainLogosObject::callMethod(const QString& authToken, @@ -277,10 +468,16 @@ QVariant PlainLogosObject::callMethodWithError(const QString& authToken, msg.method = methodName.toStdString(); msg.args = qvariantListToRpcList(args); + const std::uint64_t callNumber = msg.id; auto fut = m_conn->sendCall(std::move(msg)); if (fut.wait_for(std::chrono::milliseconds(timeoutMs)) != std::future_status::ready) { qWarning() << "PlainLogosObject::callMethod: timeout for" << methodName; + // Withdraw the registration this call left in the connection. The sync + // path has the same orphan the async one does — nothing erases a + // pending entry whose reply never comes — and the promise behind it + // holds a future nobody will ever read again. + m_conn->cancelPending(callNumber); if (err) *err = logos::callErrorTimeout(m_objectName, methodName.toStdString(), timeoutMs); @@ -333,33 +530,51 @@ void PlainLogosObject::subscribeToCompletions() { // Reuse the normal event subscription path (tracked in m_subs, so // disconnectEvents() tears it down). The handler fires on the connection's - // IO thread; it buffers the result and wakes any waiter. + // IO thread. // - // It captures a weak_ptr to the RENDEZVOUS and nothing else — in particular - // NOT `this`. That unsubscribe is real (RpcConnection::sendUnsubscribe erases - // the entry under the connection's mutex) but it is not enough on its own: + // It captures a weak_ptr to CallState and nothing else — in particular NOT + // `this`. That unsubscribe is real (RpcConnection::sendUnsubscribe erases the + // entry under the connection's mutex) but it is not enough on its own: // dispatchIncoming copies the handler out under that mutex and invokes it // with the mutex dropped, so an erase racing an already-copied handler // changes nothing about the invocation in flight, and nothing joins the io - // thread the way stopAndJoinWaiters() joins the waiters. With `this` - // captured, a completion arriving across a release() wrote into freed memory - // — see test_plain_completion_sub_lifetime.cpp. + // thread. With `this` captured, a completion arriving across a release() + // wrote into freed memory — see test_plain_completion_sub_lifetime.cpp. // // weak, not shared, deliberately: locking is what keeps the block alive for // the length of one callback, and failing to lock is what makes a handler // that outlives its owner — for this reason or any future one — a no-op // instead of an append to a map nobody will ever drain. - std::weak_ptr weak = m_completion; + std::weak_ptr weak = m_state; onEvent(logos::callCompleteEvent(), [weak](const QString&, const QVariantList& data) { if (data.size() != 2) return; - const std::shared_ptr state = weak.lock(); - if (!state) return; // the object that owned this rendezvous is gone + const std::shared_ptr st = weak.lock(); + if (!st) return; // the handle and its state are both gone const QString callId = data.at(0).toString(); + + std::shared_ptr call; { - std::lock_guard g(state->mu); - state->completions[callId] = data.at(1); + std::lock_guard g(st->mu); + auto it = st->deferred.find(callId); + if (it != st->deferred.end()) { + call = it->second; + st->deferred.erase(it); + } else { + // Nobody is waiting on it yet: either a SYNCHRONOUS caller is + // about to park on it, or it arrived before its own sentinel + // was recorded. Buffer it, exactly as before. + st->completions[callId] = data.at(1); + } } - state->cv.notify_all(); + if (call) { + // Resolves the async call HERE, on the io thread — but deliver() + // only takes a flag, drops two map entries and posts, so the user's + // callback still runs on the Qt loop. Called with st->mu released: + // deliver() takes it. + call->deliver(data.at(1), logos::CallError{}); + return; + } + st->cv.notify_all(); }); } @@ -367,34 +582,33 @@ QVariant PlainLogosObject::awaitCompletion(const QString& callId, int timeoutMs, const QString& methodName, logos::CallError* err) { - // Only ever reached from a thread that keeps this object alive — the sync - // caller in callMethodWithError, or a waiter thread, which is joined before - // the object dies. So `this` is safe here; it is the CONNECTION's handler, - // on the io thread, that is not, which is why the rendezvous the two share - // outlives neither of them by accident. - const std::shared_ptr state = m_completion; - std::unique_lock lk(state->mu); - const auto effectiveMs = timeoutMs > 0 ? timeoutMs : 30000; + // A LOCAL SHARE of the state, held for the whole wait. This function only + // ever runs on the SYNCHRONOUS caller's own thread, so that caller cannot + // be releasing the handle underneath it — but taking the share costs one + // atomic increment and removes the question entirely. + const std::shared_ptr st = m_state; + std::unique_lock lk(st->mu); + const auto effectiveMs = timeoutMs > 0 ? timeoutMs : kDeferredFallbackMs; const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(effectiveMs); - // Unlike the future wait this one is interruptible by construction: widen - // the predicate, and stopWaiters()' notify_all does the rest. No slicing, so - // no latency floor at all here — a stop wakes this wait immediately. - state->cv.wait_until(lk, deadline, [&] { - return state->completions.count(callId) > 0 - || m_stopping.load(std::memory_order_relaxed); + // Interruptible by construction: widen the predicate, and + // stopAndCancelCalls()' notify_all does the rest. No slicing, so no latency + // floor at all here — a stop wakes this wait immediately. + st->cv.wait_until(lk, deadline, [&] { + return st->completions.count(callId) > 0 + || st->stopping.load(std::memory_order_relaxed); }); - // A completion that actually landed beats a concurrent stop — the same rule - // the future wait follows (see waitForResult): there is a real answer in - // hand, so hand it over rather than manufacture an error. - const auto it = state->completions.find(callId); - if (it != state->completions.end()) { + // AN ANSWER ALREADY IN HAND BEATS A CONCURRENT STOP: there is a real result + // here, so hand it over rather than manufacture a failure that did not + // happen. Callers re-acquire, retry and log on transport_error. + const auto it = st->completions.find(callId); + if (it != st->completions.end()) { const QVariant result = it->second; - state->completions.erase(it); + st->completions.erase(it); return result; } - if (m_stopping.load(std::memory_order_relaxed)) { + if (st->stopping.load(std::memory_order_relaxed)) { qWarning() << "PlainLogosObject: deferred call" << callId << "abandoned — object released while it was in flight"; if (err) @@ -408,41 +622,6 @@ QVariant PlainLogosObject::awaitCompletion(const QString& callId, int timeoutMs, return QVariant(); } -namespace { - -// Hand `callback(result)` over to the Qt event loop so PlainLogosObject's -// async path matches LogosObject's interface contract: callbacks are -// always delivered on a subsequent event-loop iteration, on the Qt -// thread, never synchronously and never racing with QObjects/UI code. -// -// Using QCoreApplication::instance() as the anchor means the queued -// invocation lands on whichever thread runs the Qt event loop in this -// process, regardless of which worker thread completed the future. -// If the application has shut down (instance() is null), we drop the -// callback rather than invoke it from an arbitrary thread. -// -// Deliberately a FREE function taking everything BY VALUE, and deliberately not -// a member: the queued lambda runs on a later event-loop iteration, which for a -// waiter cancelled by teardown is after the PlainLogosObject is already gone. -// Nothing it touches may belong to the object — which is why the waiter copies -// objectName/method up front instead of reading m_objectName from inside here. -// Do not give this a `this`; delivering during teardown would become the -// use-after-free that joining the waiters exists to prevent. -void postToQtEventLoop(PlainLogosObject::AsyncResultErrorCallback callback, - QVariant result, logos::CallError err) -{ - QCoreApplication* app = QCoreApplication::instance(); - if (!app) return; - QMetaObject::invokeMethod(app, - [callback = std::move(callback), result = std::move(result), - err = std::move(err)]() mutable { - callback(result, err); - }, - Qt::QueuedConnection); -} - -} // anonymous namespace - void PlainLogosObject::callMethodAsync(const QString& authToken, const QString& methodName, const QVariantList& args, @@ -484,173 +663,129 @@ void PlainLogosObject::callMethodAsyncWithError(const QString& authToken, msg.method = methodName.toStdString(); msg.args = qvariantListToRpcList(args); - auto fut = std::make_shared>( - m_conn->sendCall(std::move(msg))); + // Copied, not read from the object later: everything below this line may + // outlive the handle. + const std::uint64_t callNumber = msg.id; + const std::string objectName = m_objectName; + const std::string method = methodName.toStdString(); - // Waiter thread is per-call but the callback hops back to the Qt - // event loop before running, so it never races with Qt objects. A - // future iteration can fold this wait into the shared Asio - // io_context (the connection already runs on it) so we don't spin - // up a thread per pending RPC. + // ── the call, as state rather than as a thread ────────────────────────── // - // The thread is JOINed — in reapFinishedWaiters() once it has finished, or - // in stopAndJoinWaiters() (destructor / release) if teardown gets there - // first — never detached: capturing `this` for awaitCompletion / - // m_stopping is only safe while the object is alive, and release() used to - // `delete this` while a waiter could still be mid-flight. - const std::string objectName = m_objectName; - const std::string method = methodName.toStdString(); - // Retire the previous calls' waiters before adding one. The waiters reap - // each other too, on their way out (see the guard below) — that is what - // drains a burst which then goes quiet, and it is why this site is no - // longer the only reaper. It still earns its keep: a waiter can only reap - // OTHERS, so the last one to finish has nobody behind it to collect it. - // Done BEFORE taking m_waiterMu because it joins, and joining under that - // lock is the shape described in reapFinishedWaiters(). - reapFinishedWaiters(); - // Register under the lock BEFORE the thread can outrun release(): a - // detach-then-push left a window where delete this raced the waiter. + // There used to be a std::thread here whose entire job was to be blockable, + // and a TODO saying to fold it into the io_context the connection already + // runs on. This is that fold. Nothing below spawns, joins, sleeps or polls. + auto call = std::make_shared(m_state, m_conn, callNumber, + objectName, method, timeoutMs, + std::move(callback)); + + // The deadline is fixed HERE, before the send, and as an absolute point — + // so neither the post onto the timer thread nor anything the io thread is + // doing can stretch what the caller asked for. + call->armTimer(AsyncCall::clock::now() + std::chrono::milliseconds(timeoutMs), + timeoutMs); + + bool refused = false; { - std::lock_guard g(m_waiterMu); - if (m_stopping.load(std::memory_order_acquire)) { - // Refuse rather than register: teardown has already swapped - // m_waiters out, so a thread pushed now would never be joined. - // - // To be honest about what this branch is: it is NOT a reachable - // window that got closed. m_stopping is raised only by teardown - // (release() / the destructor), so a thread that can read it as - // true here is already calling a method on an object whose - // destructor is running — this very load is the use-after-free, and - // nothing inside this function can repair that. Reproduced as a - // SIGSEGV, on this branch and on its parent alike. It is kept - // because it costs one predictable branch on a path that already - // does a socket write, and because failing this way — one callback, - // with the same error a cancelled call gets — is strictly better - // than pushing a thread nobody will ever join, should some future - // caller of stopWaiters() make the state legitimately observable. - postToQtEventLoop(std::move(callback), QVariant(), - callErrorReleased(objectName, method)); + std::lock_guard g(m_state->mu); + // Same branch, same honesty as before the fold: stopping is raised only + // by teardown, so a caller that can read it as true here is already + // calling a method on an object whose destructor is running — this very + // load is the use-after-free, and nothing in this function can repair + // that. It is kept because failing this way — one callback, with the + // error a cancelled call gets — is strictly better than registering a + // call nobody will ever cancel. + // + // What HAS changed is the blast radius if it ever became reachable: the + // state it would leak an entry into is shared-owned and would simply + // outlive the handle, instead of being a thread nobody joins. + if (m_state->stopping.load(std::memory_order_acquire)) + refused = true; + else + m_state->inflight.emplace(callNumber, call); + } + // OUTSIDE the lock, and that is not a stylistic preference: deliver() takes + // CallState::mu to leave the registries, and this mutex is not recursive. + // Delivering from inside the scope above self-deadlocks — on the one branch + // whose whole purpose is to fail gracefully. + if (refused) { + call->deliver(QVariant(), callErrorReleased(objectName, method)); + return; + } + + // The reply. Handed over by RpcConnection as it arrives, on its strand for + // the normal path — and on an arbitrary thread from fail(), or inline right + // here if the connection is already stopped. All three are fine: every exit + // below funnels into AsyncCall::deliver, whose CAS makes the first one win + // and which hops to the Qt loop rather than running the user's callback on + // whatever stack it happens to be on. + std::weak_ptr weakState = m_state; + m_conn->sendCallAsync(std::move(msg), [call, weakState](ResultMessage res) { + if (!res.ok) { + call->deliver(QVariant(), + logos::callErrorFromWire(call->objectName, res.errCode, + res.err)); + return; + } + QVariant value = rpcValueToQVariant(res.value); + QString callId; + if (!logos::isPendingCallSentinel(value, &callId)) { + call->deliver(std::move(value), logos::CallError{}); return; } - const std::uint64_t waiterId = m_nextWaiterId++; - std::thread waiter([this, waiterId, objectName, fut, timeoutMs, methodName, method, - callback = std::move(callback)]() mutable { - // Everything reached through `this` below (m_stopping, - // awaitCompletion's rendezvous mutex and map) is safe only - // because this thread is joined before the object dies — by the - // reaper if it finishes first, by stopAndJoinWaiters() otherwise. - // Everything handed to postToQtEventLoop is a COPY, because that - // delivery happens after this thread has returned — i.e. possibly - // after the object is gone. Keep it that way. - // - // Declared FIRST so it destructs LAST: publishing this waiter's id - // is what permits somebody else to join and drop it, so it must - // come after every access to the object, on every exit path - // (four returns below, plus anything that throws). What runs after - // it is the unwinding of the captures above, none of which belongs - // to the object: a string, a shared_ptr to the call's future, and a - // callback that has already been moved out. - // - // It REAPS BEFORE IT PUBLISHES, and that order is the safety - // argument rather than a stylistic choice. Two reasons, one of - // which the test suite demonstrates: - // - // * Publishing is what makes a waiter joinable BY ANOTHER WAITER. - // Reaping first keeps that relation one-way — unpublished - // threads join published ones, published ones join nobody — so - // it cannot contain a cycle. Inverted, two waiters that publish - // in the same instant can each take the other's thread out of - // m_waiters and then join it. Both are already out of the - // registry, so teardown does not even wait for them; here - // pthread_join detects the cycle and throws out of - // reapFinishedWaiters, whose half-drained vector then destroys - // a still-joinable thread — std::terminate. Re-measured over a - // longer run than the 5/5 an earlier commit message claimed: - // with the two lines below swapped, ReapingRacesPublishingWith- - // outDeadlocking aborts the process 12 runs in 15. It is a - // race, so it is a probabilistic detector and a single green - // run of it proves nothing. - // * Until it publishes, this waiter is still in m_waiters, so a - // concurrent teardown joins it and the object cannot be - // destroyed under the reap. After publishing, a reaper can take - // its thread out of the map and release() can `delete this`, - // and a reaper running on the CALLER's thread (the spawn path - // above) is one teardown neither knows about nor waits for — so - // the touch of m_waiterMu would land on freed memory. That one - // needs a caller still issuing calls while another thread - // releases, which this class already treats as caller-side UB, - // so no test can provoke it without being red on correct code. - // BE PRECISE ABOUT WHAT THAT MAKES THIS: an invariant the design - // rests on, NOT a live use-after-free waiting to be hit. Under - // supported use every waiter is still joined transitively — one - // leaves m_waiters only via teardown (which joins it) or via a - // reaper, and that reaper is either another waiter, still - // registered itself because it reaps before it publishes, or the - // spawn path, whose join finishes before the call returns. The - // only reaper nobody waits for is that spawn path racing a - // release(), i.e. the caller-side UB above. - // test_plain_waiter_publish_is_last.cpp therefore stops trying - // to provoke it and OBSERVES the accesses instead: it guards the - // object's non-registry state with mprotect while a waiter runs, - // and baits the registry with an entry planted while the waiter - // is parked mid-join. It pins the rule against future edits — - // the TODO above moves where reaping happens — rather than - // closing an open hole. - // - // Reaping here at all is what makes the retention bound hold for a - // module that bursts and then goes quiet: the spawn-path reaper - // only runs if another call ever comes. - struct FinishOnExit { - PlainLogosObject* self; - std::uint64_t id; - ~FinishOnExit() - { - self->reapFinishedWaiters(); // others, never itself - self->publishFinishedWaiter(id); // strictly last - } - } finishOnExit{this, waiterId}; - const WaitOutcome outcome = waitForResult(*fut, timeoutMs, m_stopping); - if (outcome == WaitOutcome::Cancelled) { - // A cancelled call still DELIVERS, exactly once. Returning - // silently here would honour the "stop fast" half and break the - // half that matters more: callMethodAsyncWithError (and - // lp_invoke_async above it) promise the callback fires exactly - // once, so a dropped one turns a bounded stall into an - // unbounded hang in every caller that awaits it. - postToQtEventLoop(std::move(callback), QVariant(), - callErrorReleased(objectName, method)); - return; + // ── the deferred ("multi") half ───────────────────────────────────── + auto st = weakState.lock(); + if (!st) { + call->deliver(QVariant(), + callErrorReleased(call->objectName, call->method)); + return; + } + + // Already resolved — by the deadline, or by teardown — while this reply + // was in flight. Filing it under `deferred` now would register a call + // that only the re-armed deadline would ever take out again (see + // deliver()). + if (call->delivered.load(std::memory_order_acquire)) return; + + QVariant buffered; + bool haveBuffered = false; + bool stopping = false; + { + std::lock_guard g(st->mu); + stopping = st->stopping.load(std::memory_order_relaxed); + // The completion can be buffered ALREADY. On one ordered connection + // it cannot be — the provider writes the sentinel result before the + // completion event, and both are decoded on the same strand in order + // — but checking costs one lookup and removes the assumption. + const auto it = st->completions.find(callId); + if (it != st->completions.end()) { + buffered = it->second; + st->completions.erase(it); + haveBuffered = true; + } else if (!stopping) { + call->callId = callId; // written under st->mu, read under it + st->deferred[callId] = call; } - if (outcome == WaitOutcome::TimedOut) { - postToQtEventLoop(std::move(callback), QVariant(), - logos::callErrorTimeout(objectName, method, - timeoutMs)); - return; - } - auto res = fut->get(); - if (!res.ok) { - postToQtEventLoop(std::move(callback), QVariant(), - logos::callErrorFromWire(objectName, res.errCode, - res.err)); - return; - } - QVariant value = rpcValueToQVariant(res.value); - // Resolve a "multi" provider's deferred completion (sentinel → wait for - // the completion event) right here on the waiter thread. This is the - // second interruptible site: a stop lands it on callErrorReleased, - // which still falls through to the single post below — one callback, - // whichever way this went. - logos::CallError err; - { - QString callId; - if (logos::isPendingCallSentinel(value, &callId)) - value = awaitCompletion(callId, timeoutMs, methodName, &err); - } - postToQtEventLoop(std::move(callback), std::move(value), std::move(err)); - }); - m_waiters.emplace(waiterId, std::move(waiter)); - } + } + if (haveBuffered) { + call->deliver(std::move(buffered), logos::CallError{}); + return; + } + if (stopping) { + call->deliver(QVariant(), + callErrorReleased(call->objectName, call->method)); + return; + } + // A SECOND full deadline, which is what the waiter thread gave it too: + // it ran the future wait for timeoutMs and then awaitCompletion for + // another timeoutMs. Preserved deliberately rather than tightened — + // changing how long a deferred call is allowed to take is a separate + // decision from removing the thread it used to take it on. + const int effective = + call->timeoutMs > 0 ? call->timeoutMs : kDeferredFallbackMs; + call->armTimer(AsyncCall::clock::now() + std::chrono::milliseconds(effective), + effective); + }); } bool PlainLogosObject::informModuleToken(const QString& authToken, @@ -721,8 +856,11 @@ QJsonArray PlainLogosObject::getMethods() msg.id = m_conn->nextId(); msg.object = m_objectName; + const std::uint64_t callNumber = msg.id; auto fut = m_conn->sendMethods(std::move(msg)); if (fut.wait_for(std::chrono::seconds(5)) != std::future_status::ready) { + // Same orphan as the sync call path, on the map next door. + m_conn->cancelPending(callNumber); return QJsonArray(); } auto res = fut.get(); @@ -738,15 +876,20 @@ void PlainLogosObject::release() // own events and drop our reference — the connection stays alive // until PlainTransportConnection itself is destroyed. // - // stopAndJoinWaiters() before delete: in-flight async waiters capture `this` - // (for awaitCompletion). Detaching them used to let release() free the - // object under a still-running waiter — and merely joining them made - // release() block for the rest of the call's timeout, so they are asked to - // stop first. Each abandoned call still delivers its callback, once, with - // callErrorReleased. Waiters that already finished were reaped as the - // calls after them were issued; this collects whatever is left. + // stopAndCancelCalls() before delete, and it BLOCKS ON NOTHING. In-flight + // calls used to be threads that captured `this`, so teardown had to ask them + // to stop and then join them — a wait slice at best, the rest of the call's + // timeout before that, and a use-after-free if they were merely detached. + // Now they are shared-owned state that no longer refers to this object at + // all, so cancelling is a flag, a sweep of the in-flight map, and one + // callback per abandoned call. + // + // That also makes release() safe to call from inside an event callback + // running on the single io thread — the reentrant shape remote_transport.cpp + // documents — which a "post a no-op onto the strand and wait for it" barrier + // could not have been: it would have deadlocked against itself. disconnectEvents(); - stopAndJoinWaiters(); + stopAndCancelCalls(); m_conn.reset(); delete this; } diff --git a/cpp/implementations/plain/plain_logos_object.h b/cpp/implementations/plain/plain_logos_object.h index 577fcf9..16c81a7 100644 --- a/cpp/implementations/plain/plain_logos_object.h +++ b/cpp/implementations/plain/plain_logos_object.h @@ -12,12 +12,15 @@ #include #include #include -#include #include #include namespace logos::plain { +// One in-flight async call. Defined in the .cpp — nothing outside needs its +// shape, and keeping it there keeps Boost.Asio out of this header. +struct AsyncCall; + // ----------------------------------------------------------------------------- // PlainLogosObject — consumer-side LogosObject backed by the plain-C++ // RPC runtime. Identical public shape to LocalLogosObject / RemoteLogosObject @@ -70,55 +73,83 @@ public: void release() override; quintptr id() const override; -private: - // The deferred ("multi") completion rendezvous, in a block that is OWNED by - // the object but does not DIE with it. +public: + // ── the shared call state ──────────────────────────────────────────────── // - // A multi provider returns a pending sentinel (logos::pendingCallKey) from - // callMethod and later pushes the real result as a logos::callCompleteEvent - // event keyed by callId. We subscribe to that event EAGERLY (before any - // call can defer) so a completion racing ahead of the waiter is buffered, - // then block the caller until the matching callId lands. The completion - // arrives on the connection's IO thread; the caller waits on another - // thread — mu/cv bridge them. + // Everything an OFF-THREAD handler can reach lives here rather than on the + // handle, and the handle's own lifetime stops mattering to those handlers. // - // WHY IT IS A SEPARATE BLOCK. The subscription handler lives in the - // RpcConnection, which is SHARED by every PlainLogosObject the connection - // hands out and outlives all of them (see release()). RpcConnection copies - // a handler out of its map under its own mutex and then invokes it with - // that mutex RELEASED — so the unsubscribe release() sends cannot reach a - // handler already in flight on the io thread, and nothing joins that - // thread. When the handler captured `this`, a completion arriving across a - // release() wrote to a freed object; reproduced as a SIGSEGV under Guard - // Malloc, in test_plain_completion_sub_lifetime.cpp. + // That split is not decoration. release() ends in `delete this`, and + // LogosObject's ABI is frozen (logos_object.h) — the handle crosses module + // boundaries as a raw pointer, so it cannot itself become shared-owned. + // But nothing a handler touches goes through the handle: no virtual, no + // id(), not even its address. So the STATE becomes shared-owned and the + // facade stays exactly as it was. Handlers hold a shared_ptr to the + // per-call AsyncCall and a weak_ptr to this block; the last one to run + // drops the last share, whenever that is. // - // Putting the rendezvous behind a shared_ptr and handing the handler a - // weak_ptr makes "no handler touches a destroyed object" true by - // construction: a handler that locks it keeps it alive for the length of - // one callback, and one that cannot lock it does nothing. Nothing else in - // this object is reachable from the handler, which is what keeps the fix - // this small. - struct CompletionRendezvous { - std::mutex mu; - std::condition_variable cv; + // This is the same block the completion-subscription lifetime fix + // introduced (it was CompletionRendezvous: mutex, condvar, completions), + // widened to carry the in-flight calls the fold moved off their threads. + // The guarantee it exists for is unchanged and is now load-bearing for + // three handlers instead of one: NO HANDLER TOUCHES A DESTROYED OBJECT, + // true by construction rather than by a barrier. + struct CallState { + std::mutex mu; + // The SYNC path's rendezvous, unchanged in kind: callMethodWithError + // still parks its own caller's thread here, because a synchronous call + // has to block someone and that someone is the caller. + std::condition_variable cv; std::map completions; + + // In-flight ASYNC calls, keyed by the call's wire id. This is the whole + // retention story on the handle now: an entry exists exactly while its + // call is outstanding and is erased by the one delivery it gets. No + // waiter registry, no publish list, no reaping, nothing that survives a + // completed call. + std::map> inflight; + // Second index over the same calls, for the ones a "multi" provider + // deferred: the completion event is keyed by the provider's callId + // string, not by our numeric id. + std::map> deferred; + + // Set once by teardown, never cleared. Written under `mu` so the + // condition-variable side cannot miss it, and atomic so the lock-free + // readers do not have to take the mutex. + std::atomic stopping{false}; }; - // Bring the completion subscription up, ONCE, and — the part that is not - // the same thing — make every other caller wait until it is actually up. +private: + // Deferred ("multi") completion rendezvous. A multi provider returns a + // pending sentinel (logos::pendingCallKey) from callMethod and later pushes + // the real result as a logos::callCompleteEvent event keyed by callId. We + // subscribe to that event EAGERLY (before any call can defer) so a completion + // racing ahead of the caller is buffered, then either resolve the waiting + // AsyncCall directly (async) or wake the parked caller (sync). // - // The window this closes is a LOST COMPLETION, not a crash. The flag used to - // be raised under the rendezvous mutex and the mutex DROPPED before the - // subscribe, so a second caller could read "subscribed", build its Call and - // put it on the wire while the Subscribe frame had not been enqueued yet. A - // "multi" provider that answers such a call quickly emits its completion - // into a subscription the host has not registered — PlainTransportHost:: - // fanOutEvent finds no sink for that connection and DROPS it — and the - // caller then waits out its full timeout for a result that was computed and - // thrown away. Measured on pristine master, four runs: 18 to 28 of 250 - // two-thread first-call rounds inverted on the wire, every one of them a - // dropped completion and a timed-out caller; 6 to 10 of 500 calls through - // the real host stack. + // The completion arrives on the connection's IO thread, and the subscription + // holds a weak_ptr to CallState — never `this`. That subscription lives in + // the RpcConnection, which is SHARED by every PlainLogosObject the + // connection hands out and outlives all of them (see release()), and + // dispatchIncoming copies the handler out under its own mutex and invokes it + // with that mutex RELEASED — so the unsubscribe release() sends cannot reach + // a handler already in flight, and nothing joins the io thread. With `this` + // captured, a completion arriving across a release() wrote to a freed + // object; reproduced as a SIGSEGV under Guard Malloc, in + // test_plain_completion_sub_lifetime.cpp. + // + // ONCE, and — the part that is not the same thing — with every other caller + // WAITING until it is actually up. The flag used to be raised under + // CallState::mu and the mutex DROPPED before the subscribe, so a second + // caller could read "subscribed", build its Call and put it on the wire + // while the Subscribe frame had not been enqueued yet. A "multi" provider + // that answers such a call quickly emits its completion into a subscription + // the host has not registered — PlainTransportHost::fanOutEvent finds no + // sink for that connection and DROPS it — and the caller then waits out its + // full timeout for a result that was computed and thrown away. Measured on + // pristine master, four runs: 18 to 28 of 250 two-thread first-call rounds + // inverted on the wire, every one a dropped completion and a timed-out + // caller; 6 to 10 of 500 calls through the real host stack. // // Ordering, once the two are serialized, is a property of asio and not of // luck: handlers posted to a strand run in the order they were posted when @@ -136,53 +167,30 @@ private: const QString& methodName = QString(), logos::CallError* err = nullptr); - // Ask every in-flight waiter to give up, then join them, then return. + // Raise the stop flag, then cancel every outstanding async call — each of + // which delivers its callback, once, with callErrorReleased — and wake the + // synchronous caller if one is parked. // - // The JOIN is what makes the waiters safe at all: they capture `this` (they - // read m_stopping and call awaitCompletion), and callMethodAsync used to - // DETACH them, so release()/delete racing an in-flight wait was a - // use-after-free. But joining alone means teardown blocks for whatever is - // left of the call's timeout — up to 20s on the protocol default — because - // a waiter has no reason to return early. Hence the stop first: it costs - // one wait slice instead, and a cancelled call still delivers its callback - // exactly once (with an error), because dropping it would turn the stall - // into a permanent hang in the caller awaiting it. - void stopAndJoinWaiters(); - // Raise the stop flag and wake anything parked on the rendezvous cv. Split - // out because the flag has to be published under the rendezvous mutex (see - // the .cpp). - void stopWaiters(); - - // Join and drop the waiters that have already FINISHED, so a handle that - // outlives its calls does not accumulate them. TWO call sites, which - // between them cover both shapes of traffic: - // - // * every async spawn — a call pays for the corpses of earlier ones; - // * every waiter as it finishes, BEFORE it publishes its own id — so a - // burst drains itself instead of parking until the next call, which for - // a module that bursts and goes quiet may never come. - // - // NOT called from stopAndJoinWaiters(): teardown joins by id-independent - // brute force and needs no published list. (It used to say otherwise here; - // it never did.) Cheap either way: a join on an already-returned thread is - // a couple of syscalls, and only ids a waiter itself published are touched. - void reapFinishedWaiters(); - // A waiter's FINAL act — see the scope guard in callMethodAsyncWithError. - // After this returns, that thread never touches the object again, which is - // what makes it safe for someone else to join and drop it. Nothing the - // waiter does may follow it, its own reap least of all. - void publishFinishedWaiter(std::uint64_t id); + // WHAT REPLACED THE JOIN. In-flight calls used to be threads that captured + // `this`, so teardown had to prove none of them was still running before + // `delete this`, and the only tool for that was joining threads it first had + // to ask to stop (a wait slice at best). Nothing captures `this` any more: a + // handler holds a shared_ptr to its AsyncCall and a weak_ptr to CallState. + // Teardown therefore waits for NOTHING — not the io thread, not a wait slice + // — which also means it cannot deadlock when release() is called from inside + // an event callback running on the single io thread (the shape + // remote_transport.cpp documents as real). It stays O(in-flight calls). + void stopAndCancelCalls(); std::string m_objectName; std::shared_ptr m_conn; std::mutex m_mu; std::vector> m_subs; - // Never null and never reseated: the object owns exactly one rendezvous for - // its whole life, and the only other references are the weak_ptr the - // subscription handler holds and whatever a handler has momentarily locked. - std::shared_ptr m_completion = - std::make_shared(); + // Never null and never reseated: the object owns exactly one state block for + // its whole life, and the only other references are the weak_ptrs its + // handlers hold and whatever one of them has momentarily locked. + std::shared_ptr m_state{std::make_shared()}; // "The Subscribe frame is on the strand." Stored with RELEASE after // subscribeToCompletions() returns and read with ACQUIRE on the fast path, // so a caller that skips the once_flag still inherits the edge that orders @@ -191,50 +199,6 @@ private: // What makes a concurrent first caller WAIT rather than sail past a flag // that has been raised but not yet honoured. The whole fix is this member. std::once_flag m_completionSubOnce; - - // The waiter registry. KEYED, not a plain vector, because a thread cannot - // join itself: a waiter can therefore never retire its own entry, and a - // vector left only one moment to clear it — teardown — so every completed - // call parked a finished-but-unjoined thread (~one page of resident memory - // each) for the whole life of the handle. The production shape is one - // cached handle per module reused for every call (logos_api_consumer.cpp), - // so that grew without bound. Now a waiter publishes its id into - // m_finishedWaiters as its last act, and both the next spawn and every - // OTHER waiter on its way out join and erase it: see reapFinishedWaiters(). - // - // Retention tracks neither call count nor peak concurrency. A burst drains - // as it completes, because each waiter reaps the ones that finished before - // it. What survives an idle handle is only what published after the last - // reap — at minimum the last waiter to finish, which by construction has - // nobody behind it to collect it. The next call, or teardown, takes those. - // - // That remainder is the size of the last exit batch, NOT a small constant, - // and nothing collects it while the handle stays idle: sampled out to 25.6s - // it does not move. What sets it is how SERIALIZED the exits are, because a - // waiter reaps and only then publishes: exits that interleave one after - // another leave 1, while a batch that becomes runnable together leaves most - // of itself, since the reaper that collects a large batch sits in its join - // loop (no lock held) while everyone behind it publishes and finds nobody to - // collect them. Measured on the drain of an 800-call burst that was fully - // outstanding before any reply — the worst case for this, and the shape - // test_plain_waiter_reaping.cpp builds deliberately — with all 800 answered - // at once: 1 every run on 10-core macOS, but 2-389 on a 6-core Linux box. It - // does not scale with the CALL COUNT, which is the claim; "1-2" was a - // one-platform reading of it, and a burst answered at any pace at all (16 at - // a time is enough, measured) leaves 1-6 on both. - // - // The real fix is still the TODO in callMethodAsyncWithError — fold the - // wait into the shared Asio io_context and have no thread per pending RPC - // at all. This makes the interim honest, it does not replace that. - std::mutex m_waiterMu; - std::map m_waiters; - std::vector m_finishedWaiters; - std::uint64_t m_nextWaiterId = 0; - // Read lock-free by the sliced future wait and under the rendezvous mutex - // by awaitCompletion's predicate; written under that same mutex so the - // condition-variable side cannot miss it. Never cleared — an object that - // has begun tearing down does not come back. - std::atomic m_stopping{false}; }; } // namespace logos::plain diff --git a/cpp/implementations/plain/rpc_connection.h b/cpp/implementations/plain/rpc_connection.h index a45f1ad..ac5af39 100644 --- a/cpp/implementations/plain/rpc_connection.h +++ b/cpp/implementations/plain/rpc_connection.h @@ -41,6 +41,40 @@ namespace logos::plain { class RpcConnectionBase { public: using ErrorHandler = std::function; + // A reply, handed over as it arrives instead of parked in a promise. + // + // Invoked AT MOST ONCE per call, from one of three places, and a caller has + // to answer for all three because they are not on the same thread: + // * the connection's strand (io thread) when the peer's Result frame is + // decoded — the normal path; + // * an arbitrary caller thread inside fail(), which sweeps every pending + // call when the connection is torn down (stop(), ~PlainTransport- + // Connection, RpcServer::stop()); + // * INLINE on the calling thread, inside sendCallAsync itself, when the + // connection is already stopped. + // It must therefore not block and must not run user code directly — see + // postToQtEventLoop in plain_logos_object.cpp. + // + // AT MOST ONCE is a property of the REGISTRATION, and it is weaker than it + // sounds. Three things contend for a registered handler — dispatchIncoming, + // fail()'s sweep and cancelPending() — and the extract-and-erase under m_mu + // lets exactly one of them have it, so no handler is ever invoked twice. + // + // What that does NOT buy is a cancel that arrives in time. dispatchIncoming + // copies the handler out under m_mu and invokes it with the mutex RELEASED, + // so a cancelPending() landing in that gap erases nothing and the handler + // runs to completion AFTER cancelPending() has already returned. A caller + // that gives up must therefore be able to absorb one more call. Both callers + // here are: + // * PlainLogosObject funnels every outcome into AsyncCall::deliver(), + // whose CAS makes the later arrival a no-op — that CAS, and nothing at + // this layer, is what makes DELIVERY to the user exactly-once; + // * sendCall()'s promise handler cannot be reached twice at all (only one + // contender ever gets it) and fulfilling a future its caller has already + // walked away from is a no-op. + // test_plain_cancel_pending_race.cpp builds that interleaving by hand rather + // than racing for it, and pins both. + using ResultHandler = std::function; virtual ~RpcConnectionBase() = default; @@ -49,8 +83,40 @@ public: virtual bool isOpen() const = 0; virtual std::future sendCall(CallMessage msg) = 0; + // The same send, completion-driven. sendCall() is now a thin wrapper over + // this one (it fulfils a promise from the handler), so there is exactly one + // registration path and the two cannot drift. + virtual void sendCallAsync(CallMessage msg, ResultHandler handler) = 0; virtual std::future sendMethods(MethodsMessage msg) = 0; + // Forget a pending Call or Methods registration whose caller has given up. + // + // THIS IS A RETENTION FIX, and it closes a hole that predates the async + // rework. m_pendingCalls / m_pendingMethods are emptied by exactly two + // events: a decoded reply carrying that id, and fail()'s teardown sweep. A + // call that is resolved by its DEADLINE and never answered is in neither, + // so its registration — a promise, or now a handler holding the caller's + // std::function — stayed in the map for the whole life of the connection. + // Measured against pristine master with a server that never answers: 8.5MB + // of resident memory over 24,000 orphaned calls, 353 bytes each, growing + // strictly linearly with the call count. And the connection outlives every + // handle it hands out, so nothing else was ever going to collect it. + // + // Erasing is the right semantic and not merely a cleanup: the caller has + // already been told the call timed out, so a reply arriving afterwards must + // be dropped, which is exactly what an absent registration does. + // + // Safe to call at any time and from any thread, including for an id that + // has already been answered (the erase simply finds nothing). Ids come from + // nextId() and are unique across BOTH maps, so one entry point covers them. + // + // BEST EFFORT AGAINST A REPLY ALREADY IN FLIGHT, and deliberately not more. + // It withdraws a REGISTRATION; it does not stop a handler dispatchIncoming + // has already taken out of the map. Returning from this is therefore not a + // guarantee of silence — see ResultHandler for who has to absorb the + // difference and how. + virtual void cancelPending(uint64_t id) = 0; + virtual void sendSubscribe(SubscribeMessage msg, std::function callback) = 0; virtual void sendUnsubscribe(UnsubscribeMessage msg) = 0; @@ -89,8 +155,15 @@ public: bool isOpen() const override { return !m_stopped.load(); } std::future sendCall(CallMessage msg) override; + void sendCallAsync(CallMessage msg, ResultHandler handler) override; std::future sendMethods(MethodsMessage msg) override; + void cancelPending(uint64_t id) override { + std::lock_guard g(m_mu); + m_pendingCalls.erase(id); + m_pendingMethods.erase(id); + } + void sendSubscribe(SubscribeMessage msg, std::function callback) override; void sendUnsubscribe(UnsubscribeMessage msg) override; @@ -131,9 +204,10 @@ private: std::deque> m_writeQueue; bool m_writing = false; - // Outgoing-pending maps + // Outgoing-pending maps. Calls hold a HANDLER rather than a promise: the + // promise is one possible handler (see sendCall), not the mechanism. std::mutex m_mu; - std::map>> m_pendingCalls; + std::map m_pendingCalls; std::map>> m_pendingMethods; using EventKey = std::pair; // object, event @@ -225,16 +299,24 @@ void RpcConnection::dispatchIncoming(AnyMessage msg) using T = std::decay_t; if constexpr (std::is_same_v) { - std::shared_ptr> p; + ResultHandler h; { std::lock_guard g(m_mu); auto it = m_pendingCalls.find(m.id); if (it != m_pendingCalls.end()) { - p = std::move(it->second); + h = std::move(it->second); m_pendingCalls.erase(it); } } - if (p) p->set_value(std::forward(m)); + // Erased under the lock BEFORE the call, so this, fail()'s sweep and + // cancelPending() cannot all get the same handler — that is what + // makes INVOCATION at-most-once at this layer, and it is the whole + // of what this layer promises. It is NOT a cancellation barrier: the + // call below runs with m_mu released, so a cancelPending() racing it + // finds the entry already gone, erases nothing, and returns while + // this handler is still running. Exactly-once DELIVERY belongs to the + // handler — see ResultHandler. + if (h) h(std::forward(m)); } else if constexpr (std::is_same_v) { std::shared_ptr> p; @@ -303,19 +385,35 @@ RpcConnection::sendCall(CallMessage msg) { auto p = std::make_shared>(); auto f = p->get_future(); + // The promise is now just one shape of handler. Everything the future path + // relied on — registration under m_mu before the write, the stopped + // early-out, fail()'s sweep — lives in sendCallAsync and is shared verbatim. + sendCallAsync(std::move(msg), [p](ResultMessage r) { + try { p->set_value(std::move(r)); } catch (...) {} + }); + return f; +} + +template +void RpcConnection::sendCallAsync(CallMessage msg, ResultHandler handler) +{ + if (!handler) return; if (m_stopped.load()) { + // Answered INLINE, on the caller's thread. That is the same shape the + // future path had (it set the promise before returning it), and it is + // why every handler in this codebase has to be non-blocking and has to + // hand user code off to the Qt loop rather than run it here. ResultMessage r; r.id = msg.id; r.ok = false; r.err = "connection stopped"; r.errCode = "TRANSPORT_CLOSED"; - p->set_value(std::move(r)); - return f; + handler(std::move(r)); + return; } { std::lock_guard g(m_mu); - m_pendingCalls[msg.id] = p; + m_pendingCalls[msg.id] = std::move(handler); } writeFrame(encodeFrame(*m_codec, AnyMessage{std::move(msg)})); - return f; } template @@ -471,8 +569,8 @@ void RpcConnection::fail(const std::string& reason) bool expected = false; if (!m_stopped.compare_exchange_strong(expected, true)) return; - // Fail every pending promise with a transport-level error. - std::map>> calls; + // Fail every pending call with a transport-level error. + std::map calls; std::map>> methods; ErrorHandler errCb; { @@ -482,10 +580,13 @@ void RpcConnection::fail(const std::string& reason) errCb.swap(m_error); m_eventCallbacks.clear(); } - for (auto& [id, p] : calls) { + for (auto& [id, h] : calls) { ResultMessage r; r.id = id; r.ok = false; r.err = reason; r.errCode = "TRANSPORT_ERROR"; - try { p->set_value(std::move(r)); } catch (...) {} + // Runs on WHATEVER THREAD called stop() — usually not the io thread. + // Handlers are written for that (see ResultHandler); the try/catch is + // the same containment the promise sweep already had. + try { h(std::move(r)); } catch (...) {} } for (auto& [id, p] : methods) { MethodsResultMessage r; r.id = id; r.ok = false; r.err = reason; diff --git a/tests/protocol/CMakeLists.txt b/tests/protocol/CMakeLists.txt index ee10849..6c15891 100644 --- a/tests/protocol/CMakeLists.txt +++ b/tests/protocol/CMakeLists.txt @@ -1,20 +1,46 @@ find_package(Qt${QT_VERSION_MAJOR} REQUIRED COMPONENTS Test) -# ── validating the detectors ──────────────────────────────────────────────── +# ── which of these tests are detectors, and how that was checked ───────────── # -# A race assertion that has never been seen to fail is not evidence, so the race -# tests in this directory are checked against the code they were written for: -# copy the test file onto the pre-fix tree and run it there. For the completion -# subscription that tree is `master` — it has both the racy ensureCompletionSub() -# and the raw-`this` handler capture — and the numbers those runs produced are -# recorded in the test files themselves. +# A race assertion that has never been seen to fail is not evidence. Three of the +# guarantees in this suite are single points of mechanism — the exactly-once CAS +# in AsyncCall::deliver(), the deadline living on its own thread rather than the +# shared io thread, and the serialization that keeps a concurrent first caller +# from outrunning the completion subscription — so each was checked by running +# the tests against code that does not have it. # -# Deliberately no build flag, no environment probe and no second implementation -# in the shipped source. An earlier draft carried both, and the cost is the same -# either way: production code grows a path whose only purpose is to be wrong. -# Running the real pre-fix code is also the stronger evidence, because it proves -# the test catches the bug that actually existed rather than a hand-written -# imitation of it. +# That check is done on the PRE-FIX TREE, not on a switch in this one: copy the +# test file onto the commit the fix replaced and run it there. For the completion +# subscription that tree is `master`, which still has both the racy +# ensureCompletionSub() and the raw-`this` handler capture; for the fold's two +# mechanisms it is a local edit to the transport, made in a throwaway checkout +# and thrown away with it. The numbers each run produced are recorded in the test +# files themselves. +# +# Deliberately not a build option, and not the environment-variable probes an +# earlier draft carried. Both have the same cost, which the getenv() version only +# made more obvious: production source ends up holding a second implementation of +# its own contract whose entire purpose is to be wrong, in a change whose entire +# purpose is correctness. Running the real pre-fix code is also better evidence, +# because it proves the test catches the bug that existed rather than a +# hand-written imitation of it. +# +# WHICH TESTS ARE DETECTORS, since it is not all of them and the difference +# matters when one goes green. Only these fail on code missing the mechanism: +# +# IoFoldTest.ReleaseRacingRepliesInFlightDeliversEachCallOnce +# IoFoldTest.DeadlineFiresOnTimeWhileTheIoThreadIsBusy +# IoFoldTest.DeadlineFiresWhileTheIoThreadIsBlockedForever +# PlainCompletionSubOrderTest.AConcurrentFirstCallCannotOutrunTheSubscription +# PlainCompletionSubOrderTest.TheSameRaceThroughTheRealHost +# PlainCancelPendingRaceTest.AnExtractedReplyRacingTeardownDeliversExactlyOnce +# +# Note what is NOT on that list: the PER-PATH exactly-once tests stay GREEN with +# the CAS removed. They are PINS, NOT DETECTORS — a call resolved once calls +# deliver() once no matter what guards it — and only racing teardown against +# replies in flight (the first entry) and the hand-built extract-then-cancel +# interleaving (the last) actually catch a doubled callback. Do not read a green +# per-path test as evidence the gate is there. add_executable(protocol_tests test_main.cpp @@ -73,33 +99,35 @@ add_executable(protocol_tests # is not silently plain-TCP-only. Exercises the deferred-completion timeout, # which needs no blocking provider and therefore no second event loop. test_call_error_qt_remote.cpp - # Teardown of a PlainLogosObject with a call still IN FLIGHT. Joining the - # per-call waiters closed a use-after-free but left release() blocking for - # the remainder of the call's timeout (up to the 20s default), because - # joinWaiters() could only join, never ask a waiter to stop. Pins all three - # halves of the fix: teardown costs one wait slice, the callback still fires - # EXACTLY ONCE on every outcome including cancellation (a waiter that just - # returns turns a stall into a hang), and the join that keeps `this` alive - # under the waiter is still there. + # Teardown of a PlainLogosObject with a call still IN FLIGHT. Written when + # in-flight calls were threads: joining them closed a use-after-free but left + # release() blocking for the remainder of the call's timeout, so they were + # asked to stop first and teardown cost one 25ms wait slice. Public API only, + # so the io_context fold did not touch it — but its BOUNDS are now loose + # rather than tight: there is no wait slice any more (0ms, measured) and no + # join, because nothing captures `this`. What it still pins exactly is the + # part that matters most: the callback fires EXACTLY ONCE on every outcome + # including cancellation, since a dropped one turns a stall into a hang. test_plain_object_teardown.cpp # The other half of the same mechanism: what a COMPLETED call leaves behind. - # A waiter cannot join itself, so while the registry was a plain vector the - # only thing that ever emptied it was teardown, and every finished call - # parked an unjoined thread (~one page of resident memory) for the life of - # the handle — unbounded under the cached-handle shape LogosAPIConsumer - # actually uses. Pins the registry not growing with call count, the - # reap-vs-publish race not deadlocking, and teardown still joining what is - # left. + # Written for the waiter registry (a map of threads plus a publish list) and + # retargeted by the io_context fold at the registry that replaced it, + # CallState::inflight. The claims are unchanged and are why it survived the + # rework: retention must not grow with call count, a burst that goes idle + # must drain with no further call, and teardown must still deliver exactly + # once. test_plain_waiter_reaping.cpp - # The ordering rule the other two rest on: publishFinishedWaiter() is a - # waiter's LAST access to the object, which is the only reason - # stopAndJoinWaiters() can return while a reaper is still mid-join on a - # waiter it has already taken out of m_waiters. No supported caller can - # provoke the use-after-free an extra access below the publish would create, - # so this observes the accesses directly: the object is placed across a page - # boundary at m_waiterMu and its non-registry page is guarded while a waiter - # runs. - test_plain_waiter_publish_is_last.cpp + # test_plain_waiter_publish_is_last.cpp — DELETED BY THE io_context FOLD. + # It pinned one rule: publishFinishedWaiter() is a waiter thread's LAST + # access to the object, which was the only reason stopAndJoinWaiters() could + # return while a reaper was still mid-join on a waiter it had already taken + # out of m_waiters. There are no waiter threads, no reaper and no publish + # list any more, so there is no ordering left to pin — the property it + # protected ("no handler touches the object after teardown") is structural + # now: handlers hold a weak_ptr to CallState and never dereference the + # handle. test_iofold.cpp OBSERVES that instead of pinning the protocol that + # used to be needed for it. + test_iofold.cpp # The OTHER thing release() leaves behind: the deferred-completion event # subscription. Its handler captured raw `this` and lives in the # RpcConnection, which is shared across every handle and outlives all of @@ -115,6 +143,13 @@ add_executable(protocol_tests # subscription the host had not registered and the completion was dropped. # Costs a caller its whole timeout and leaves nothing behind — see the file. test_plain_completion_sub_order.cpp + # cancelPending() and the two things it is NOT. dispatchIncoming copies a + # handler out under the connection mutex and invokes it unlocked, so a + # cancel arriving in that gap stops nothing — at-most-once INVOCATION is the + # extract-and-erase, and exactly-once DELIVERY is AsyncCall::deliver()'s CAS. + # Constructs the interleaving by hand rather than racing for it, and covers + # the OTHER handler shape (the promise sendCall registers) too. + test_plain_cancel_pending_race.cpp # Component tests that moved here with their code (from logos-cpp-sdk) test_token_manager.cpp test_mock_store.cpp diff --git a/tests/protocol/test_iofold.cpp b/tests/protocol/test_iofold.cpp new file mode 100644 index 0000000..315dfcd --- /dev/null +++ b/tests/protocol/test_iofold.cpp @@ -0,0 +1,1514 @@ +// The io_context fold: an async call is state plus a deadline, not a thread. +// This file is the evidence for that, and the replacement for +// test_plain_waiter_publish_is_last.cpp, whose subject (a waiter thread's +// publish ordering) no longer exists. +// +// WHAT IS PINNED HERE, and why each one needs its own test: +// +// 1. NO THREAD PER PENDING RPC — the point of the exercise. Measured with the +// OS thread count of the PROCESS, against calls genuinely parked in a +// provider that will not answer. Pre-fold this is +1 per in-flight call. +// +// 2. EXACTLY ONCE, on every path, counted PER CALL. This is the guarantee most +// at risk from the rework, because its mechanism changed: it used to be +// structural (one thread, one function body, and a join proving it had +// finished) and is now a single CAS that three independent callers race for +// — the reply handler, the deadline, and teardown. The plain outcomes +// (normal / deferred / timeout / cancelled) are WEAK detectors of that: a +// call resolved once calls deliver() once, and stays green with the CAS +// removed. Measured, not assumed — so the strong one is a separate test +// that puts teardown and a stream of arriving replies on the same calls. +// +// 3. THE DEADLINE IS NOT HOSTAGE TO THE IO THREAD. The version of this work +// that put the per-call timer on the connection's strand had exactly one +// regression, and this was it: IoContextPool runs ONE thread for the whole +// process and this transport delivers user onEvent callbacks INLINE on it, +// so an event handler that made a 2000ms call delayed a 200ms deadline on a +// DIFFERENT connection to 2003ms. Both shapes are pinned — a busy io thread +// and a permanently blocked one. +// +// 4. RETENTION IS BOUNDED BY WHAT IS IN FLIGHT, on both sides: the handle's +// CallState registries AND the connection's pending-call map, which nothing +// used to erase for a call resolved by its deadline. +// +// 5. NO USE-AFTER-FREE, which is what replaced the join. Nothing waits for the +// io thread; instead no handler can reach the handle. +// +// HOW THE DETECTORS ARE VALIDATED. By running them against a transport that +// does not have the mechanism, in a throwaway checkout — two edits, neither of +// which is carried in this tree: +// +// (a) AsyncCall::claim() stores `delivered` and returns true unconditionally +// instead of compare-exchanging it, and takeCallback() returns a COPY of +// the callback instead of swapping it out. Both halves have to go: they +// are independently sufficient. +// (b) deadlineContext() returns IoContextPool::shared().ioContext() — the +// connections' own thread — instead of DeadlineService::shared() +// .context(). That is the rejected design named in the DeadlineService +// comment, so this measures the regression that class exists to prevent. +// +// A build option that did this from inside the shipped source was tried and +// removed: it left a second, knowingly wrong implementation of the exactly-once +// gate in the production translation unit, which is not a thing a correctness +// change gets to ship. (An earlier draft used getenv() probes and was worse in +// the same way.) Doing it as a local edit costs one throwaway build and proves +// the same thing. The related sub-order detector needs no edit at all — it goes +// red on pristine master; see test_plain_completion_sub_order.cpp. +// +// The numbers those runs produced, on an aarch64-darwin box: +// +// ReleaseRacingRepliesInFlightDeliversEachCallOnce (a) 6-16 double +// deliveries per +// 10,000 calls, 4 runs +// DeadlineFiresOnTimeWhileTheIoThreadIsBusy (b) 200ms deadline fires +// at 2002ms +// DeadlineFiresWhileTheIoThreadIsBlockedForever (b) never fires at all +// +// A green run of any of those against the stripped transport means the test is +// not exercising what it claims to, and is a bug in the test. Two candidates +// were REJECTED as exactly-once detectors on exactly that ground — see the +// comment on ReleaseRacingRepliesInFlightDeliversEachCallOnce. +// +// Everything runs against a live in-process PlainTransportHost over real TCP. + +#include + +#include "logos_async_dispatch.h" +#include "logos_call_error.h" +#include "logos_object.h" +#include "logos_provider_interface.h" +#include "logos_transport_config.h" +#include "module_proxy.h" + +#include "plain_logos_object.h" +#include "plain_transport_connection.h" +#include "plain_transport_host.h" + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifdef __APPLE__ +#include +#else +#include +#include +#endif + +using namespace logos::plain; + +namespace { + +// Live OS threads in this process. The measurement has to be of the PROCESS, +// not of anything the object reports about itself: the claim is that pending +// calls stopped costing threads, and an object-level counter would only be +// restating the implementation. +int liveThreads() +{ +#ifdef __APPLE__ + thread_act_array_t list = nullptr; + mach_msg_type_number_t n = 0; + if (task_threads(mach_task_self(), &list, &n) != KERN_SUCCESS) return -1; + for (mach_msg_type_number_t i = 0; i < n; ++i) + mach_port_deallocate(mach_task_self(), list[i]); + vm_deallocate(mach_task_self(), reinterpret_cast(list), + n * sizeof(thread_act_t)); + return static_cast(n); +#elif defined(__linux__) + std::ifstream st("/proc/self/status"); + std::string line; + while (std::getline(st, line)) { + if (line.rfind("Threads:", 0) == 0) + return std::atoi(line.c_str() + 8); + } + return -1; +#else + return -1; // the assertions below are skipped where this is unavailable +#endif +} + +// ── the provider's own worker threads, and why they are counted ────────────── +// +// A "multi" provider answers with a pending sentinel and completes the call +// LATER, from a worker of its own — that is the whole point of `defer` below, +// and it is what makes the completion arrive on the consumer's io thread instead +// of inline in the reply. The worker calls the EventCallback ModuleProxy handed +// the provider in setEventListener(), and that listener captures `this` RAW +// (module_proxy.cpp): its first act is +// QMetaObject::invokeMethod(this, …, Qt::QueuedConnection), which dereferences +// the QObject. +// +// This fixture DELETES that proxy. The worker was spawned detached, so nothing +// proved it had finished, and `delete m_proxy` followed the last spawn by a +// couple of milliseconds — the fixture drains the CALLER's side of each round +// (pumpUntilTotal) but a release()d call is answered by teardown, so the round +// can be over before the provider has even run, leaving a backlog of `defer` +// calls that the proxy's thread is still working through while ~LiveHost runs. +// +// Reproduced on Linux (aarch64, Qt 6.9.2, gcc 14.3), `nix build .#tests` +// artifact, `--gtest_filter=IoFoldTest.*`, five concurrent copies — 6 of 50 runs +// on the tree this provider arrived on, 10 of 50 once the fixtures started +// destroying their host on the proxy's thread (which lets the backlog RUN instead +// of discarding it with QThread::quit(), so it widens this window rather than +// opening it). Every one of them with two to five worker threads standing in the +// same frame: +// +// Thread "QThread" received signal SIGSEGV +// QObject::thread() const +// QMetaObject::invokeMethodImpl(QObject*, …) +// ModuleProxy::ModuleProxy(...):: +// module_proxy.cpp:37 +// std::function&)>::operator() +// OmniProvider::callMethod(...):: this file, in defer +// std::thread::_State_impl<…>::_M_run() +// +// The corpse is left by the test that spawned the worker and lands in whichever +// test runs NEXT, so it only appears in the WHOLE-BINARY run — the CI step that +// runs ./result/bin/protocol_tests. Under ctest every test is its own process, so +// the worker dies with the process that owned the proxy and there is nothing left +// to fault; `nix build .#tests` is green on the same tree, which is exactly how +// this hid. +// +// A COUNT, NOT A JOIN, deliberately. Making the workers joinable would hold their +// 8MB stacks until something reaped them — 400 outstanding in +// NormalAndDeferredCompletionsDeliverExactlyOnceAtVolume — and reaping from the +// dispatch thread would block the very thread `defer` exists to free. So they +// stay detached and the provider counts them; ~LiveHost waits for the count to +// reach zero at the one point where it is final. +// +// The same shape exists in test_plain_completion_sub_order.cpp +// (InstantMultiModule) and test_concurrent_dispatch.cpp, which additionally read +// the callback member through a captured `this`. Neither has been observed to +// fault and neither is touched here. +class EmitterGate { +public: + // Raised on the DISPATCH thread, BEFORE the worker is spawned. Raising it + // inside the worker instead would leave drain() free to pass through the gap + // between the spawn and the worker's first instruction. + void enter() + { + std::lock_guard g(m_mu); + ++m_live; + } + + // The worker's LAST act. notify_all() runs with m_mu HELD, and that is the + // whole of the guarantee: a drain() woken by it has to re-acquire m_mu, which + // it cannot do until this thread has released it. So once drain() returns, no + // worker touches this gate — or the provider that holds it, or the proxy — + // ever again. + void leave() + { + std::lock_guard g(m_mu); + if (--m_live == 0) m_cv.notify_all(); + } + + // Unbounded on purpose: the caller is a fixture already sitting on + // QThread::wait() with no timeout one line above, and a bounded wait here + // would hand back exactly the "probably finished" that the bare detach had. + void drain() + { + std::unique_lock lk(m_mu); + m_cv.wait(lk, [this] { return m_live == 0; }); + } + +private: + std::mutex m_mu; + std::condition_variable m_cv; + int m_live = 0; +}; + +// enter() then spawn, so the count is up before the worker exists; leave() from a +// scope guard, so an emitter that throws cannot strand the count and wedge +// drain() forever. `gate` is captured by reference because it is a member of the +// provider, and the drain is precisely what keeps that alive long enough. +template +void spawnGatedEmitter(EmitterGate& gate, Fn fn) +{ + gate.enter(); + try { + std::thread([&gate, fn = std::move(fn)]() mutable { + struct Leave { + EmitterGate* g; + ~Leave() { g->leave(); } + } leave{&gate}; + fn(); + }).detach(); + } catch (...) { + gate.leave(); + throw; + } +} + +// One provider covering every outcome the fold has to preserve: +// ping — answers immediately (normal completion) +// block — parks until letGo() (in-flight; timeout; cancellation) +// defer — "multi": returns the pending sentinel and completes it later +// sink — "multi": returns the sentinel and NEVER completes it +// slowsink — "multi": the sentinel itself arrives after the caller's deadline +// fire — emits a user event, which this transport delivers INLINE on the +// consumer's io thread +class OmniProvider : public LogosProviderObject { +public: + QVariant callMethod(const QString& method, const QVariantList& args) override + { + if (method == QLatin1String("ping")) return args.value(0, QVariant(1)); + if (method == QLatin1String("block")) { + std::unique_lock lk(m_mu); + m_cv.wait(lk, [this] { return m_released; }); + return QVariant(42); + } + if (method == QLatin1String("fire")) { + if (m_eventCb) m_eventCb(QStringLiteral("tick"), QVariantList{ QVariant(1) }); + return QVariant(true); + } + if (method == QLatin1String("slowsink")) { + // The sentinel itself arrives LATE — after the caller's deadline + // has already elapsed and the call has been resolved as a timeout. + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + QVariantMap pending; + pending[logos::pendingCallKey()] = QStringLiteral("late-%1").arg( + static_cast(m_counter.fetch_add(1))); + return pending; + } + if (method == QLatin1String("sink")) { + QVariantMap pending; + pending[logos::pendingCallKey()] = QStringLiteral("never-%1").arg( + static_cast(m_counter.fetch_add(1))); + return pending; + } + if (method == QLatin1String("defer")) { + const int delayUs = args.value(0).toInt(); + const QString callId = QStringLiteral("cid-%1").arg( + static_cast(m_counter.fetch_add(1))); + auto cb = m_eventCb; + // The completion is pushed from a worker, which is what a real + // "multi" provider does and what makes the event arrive on the + // consumer's io thread rather than inline in the reply. + // + // GATED rather than plain-detached, because `cb` reaches the + // ModuleProxy through a raw `this` and this fixture deletes that + // proxy: the gate is what lets ~LiveHost prove no worker is left + // before it does. Reproduced SIGSEGV and full reasoning at + // EmitterGate above. + spawnGatedEmitter(m_emitters, [cb, callId, delayUs]() { + if (delayUs > 0) + std::this_thread::sleep_for(std::chrono::microseconds(delayUs)); + if (cb) cb(logos::callCompleteEvent(), + QVariantList{ callId, QVariant(7) }); + }); + QVariantMap pending; + pending[logos::pendingCallKey()] = callId; + return pending; + } + return QVariant(); + } + + void letGo() + { + { + std::lock_guard g(m_mu); + m_released = true; + } + m_cv.notify_all(); + } + + // Wait out every worker `defer` has spawned. ~LiveHost calls this once the + // proxy's event loop has stopped — the point after which no further + // callMethod can be dispatched, so the set of workers is final — and before + // the proxy those workers emit into is deleted. + void drainEmitters() { m_emitters.drain(); } + + // The same wait, as this provider's OWN invariant: a worker's last act is + // EmitterGate::leave() on a member of this object, so none may outlive it. + // The destructor BODY runs before any member is destroyed, so m_emitters is + // still alive here. This does NOT cover the proxy — by the time a provider is + // destroyed its owner has usually deleted that already — which is why + // ~LiveHost has to drain too, and earlier. + ~OmniProvider() override { m_emitters.drain(); } + + QJsonArray getMethods() override { return QJsonArray{}; } + bool informModuleToken(const QString&, const QString&) override { return true; } + void setEventListener(EventCallback cb) override { m_eventCb = std::move(cb); } + void init(void*) override {} + QString providerName() const override { return QStringLiteral("omni"); } + QString providerVersion() const override { return QStringLiteral("1.0.0"); } + +private: + std::mutex m_mu; + std::condition_variable m_cv; + bool m_released = false; + EventCallback m_eventCb; + std::atomic m_counter{0}; + EmitterGate m_emitters; +}; + +QCoreApplication* ensureApp() +{ + static int argc = 0; + static char* argv[] = { nullptr }; + if (!QCoreApplication::instance()) + new QCoreApplication(argc, argv); + return QCoreApplication::instance(); +} + +class LiveHost { +public: + LiveHost() + { + LogosTransportConfig cfg; + cfg.protocol = LogosProtocol::Tcp; + cfg.host = "127.0.0.1"; + cfg.port = 0; // ephemeral + m_host = std::make_unique(cfg); + m_started = m_host->start(); + + m_proxy = new ModuleProxy(&m_provider); + m_proxy->saveToken(QStringLiteral("origin"), QStringLiteral("live-token")); + m_thread = new QThread; + m_proxy->moveToThread(m_thread); + m_thread->start(); + + m_published = m_host->publishObject("omni_module", m_proxy); + const QString endpoint = m_host->endpoint(); + m_port = endpoint.mid(endpoint.lastIndexOf(':') + 1).toUShort(); + } + + ~LiveHost() + { + m_provider.letGo(); + QCoreApplication::processEvents(QEventLoop::AllEvents, 200); + m_host.reset(); + QCoreApplication::processEvents(QEventLoop::AllEvents, 50); + m_thread->quit(); + m_thread->wait(); + // The proxy's event loop has stopped, so no queued call can reach the + // provider any more and the set of `defer` workers is now FINAL. Wait + // them out before deleting the proxy they emit into — one of them + // outliving this line is the SIGSEGV recorded at EmitterGate above. + m_provider.drainEmitters(); + delete m_proxy; + delete m_thread; + } + + bool ok() const { return m_started && m_published && m_port != 0; } + uint16_t port() const { return m_port; } + OmniProvider& provider() { return m_provider; } + +private: + OmniProvider m_provider; + std::unique_ptr m_host; + ModuleProxy* m_proxy = nullptr; + QThread* m_thread = nullptr; + bool m_started = false; + bool m_published = false; + uint16_t m_port = 0; +}; + +std::unique_ptr connectTo(uint16_t port) +{ + LogosTransportConfig cfg; + cfg.protocol = LogosProtocol::Tcp; + cfg.host = "127.0.0.1"; + cfg.port = port; + auto conn = std::make_unique(cfg); + if (!conn->connectToHost()) return nullptr; + return conn; +} + +LogosObjectErrorChannel* channelFor(LogosObject* obj) +{ + return dynamic_cast(obj); +} + +// Reads the registries through the explicit-instantiation access hole +// ([temp.spec] does not check access on the template arguments of an explicit +// instantiation), so the code under test is observed exactly as it ships — no +// friend, no test-only accessor, no `#define private public`. +template +struct Rob { + friend typename Tag::type get(Tag) { return Member; } +}; + +struct StateTag { + using type = std::shared_ptr PlainLogosObject::*; + friend type get(StateTag); +}; +template struct Rob; + +struct ConnTag { + using type = std::shared_ptr PlainLogosObject::*; + friend type get(ConnTag); +}; +template struct Rob; + +using TcpConn = RpcConnection; + +struct PendingCallsTag { + using type = std::map TcpConn::*; + friend type get(PendingCallsTag); +}; +template struct Rob; + +struct ConnMuTag { + using type = std::mutex TcpConn::*; + friend type get(ConnMuTag); +}; +template struct Rob; + +struct Registries { + size_t inflight; + size_t deferred; + size_t completions; + size_t pendingOnConnection; +}; + +Registries registries(PlainLogosObject* obj) +{ + Registries r{0, 0, 0, 0}; + { + auto& st = obj->*get(StateTag{}); + std::lock_guard g(st->mu); + r.inflight = st->inflight.size(); + r.deferred = st->deferred.size(); + r.completions = st->completions.size(); + } + auto& base = obj->*get(ConnTag{}); + if (auto* c = dynamic_cast(base.get())) { + std::lock_guard g(c->*get(ConnMuTag{})); + r.pendingOnConnection = (c->*get(PendingCallsTag{})).size(); + } + return r; +} + +// Per-call delivery counts. Both 0 and 2 are failures, and they are counted per +// call rather than in aggregate — a double delivery on one call plus a dropped +// one on another balances out in a total. +struct Deliveries { + explicit Deliveries(int n) : counts(n) {} + std::vector> counts; + std::atomic total{0}; + + std::mutex mu; + std::string lastCode; + QVariant lastValue; + + void record(int i, QVariant v, const logos::CallError& e) + { + { + std::lock_guard g(mu); + lastCode = e.code; + lastValue = std::move(v); + } + counts[i].fetch_add(1); + total.fetch_add(1); + } + std::string code() { std::lock_guard g(mu); return lastCode; } + int worst() const + { + int w = 0; + for (const auto& c : counts) w = std::max(w, c.load()); + return w; + } + int missing() const + { + int m = 0; + for (const auto& c : counts) if (c.load() == 0) ++m; + return m; + } +}; + +void pump(int ms) +{ + QElapsedTimer t; + t.start(); + while (t.elapsed() < ms) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); +} + +void pumpUntilTotal(Deliveries& d, int target, int budgetMs) +{ + QElapsedTimer t; + t.start(); + while (d.total.load() < target && t.elapsed() < budgetMs) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); +} + +const char* kToken = "live-token"; + +} // namespace + +class IoFoldTest : public ::testing::Test { +protected: + void SetUp() override { ensureApp(); } +}; + +// ── 1. no thread per pending RPC ──────────────────────────────────────────── +// +// 32 calls parked in a provider that will not answer. Pre-fold each of those is +// an OS thread sitting in a sliced future wait — measured at exactly +1 per +// in-flight call. The claim here is that the same 32 calls cost none. +// +// The bound is 2 rather than 0 on purpose: Qt is free to service the connection +// from a pool thread of its own, and the two singleton threads this transport +// owns (the io worker and the deadline clock) are lazily created, which is what +// the warm-up call below is for. What must not happen is growth WITH the call +// count — with 32 in flight, anything at or near 32 is the old design. +TEST_F(IoFoldTest, PendingCallsDoNotCostThreads) +{ + if (liveThreads() < 0) GTEST_SKIP() << "no thread counter on this platform"; + + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + // One warm-up call, completed, so the io worker, the deadline thread and any + // Qt pool threads exist before the baseline is taken. Otherwise the fold gets + // blamed for threads that lazy initialisation created. + { + Deliveries warm(1); + ch->callMethodAsyncWithError(kToken, QStringLiteral("ping"), + QVariantList{ QVariant(1) }, 5000, + [&warm](QVariant v, const logos::CallError& e) { + warm.record(0, std::move(v), e); + }); + pumpUntilTotal(warm, 1, 10000); + ASSERT_EQ(warm.total.load(), 1); + } + pump(150); + + const int base = liveThreads(); + + constexpr int kInFlight = 32; + Deliveries d(kInFlight); + for (int i = 0; i < kInFlight; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 20000, + [&d, i](QVariant v, const logos::CallError& e) { + d.record(i, std::move(v), e); + }); + } + // Long enough for every call to be registered and genuinely outstanding. + pump(400); + const int parked = liveThreads(); + ASSERT_EQ(d.total.load(), 0) << "the provider answered; nothing was in flight"; + + std::cout << " " << kInFlight << " calls PARKED in the provider -> threads " + << base << " -> " << parked << " (delta " << (parked - base) << ")" + << std::endl; + + EXPECT_LE(parked - base, 2) + << "in-flight calls are still costing threads: +" << (parked - base) + << " for " << kInFlight << " pending calls"; + + QElapsedTimer timer; + timer.start(); + obj->release(); + const qint64 releaseMs = timer.elapsed(); + + pumpUntilTotal(d, kInFlight, 5000); + pump(200); + const int after = liveThreads(); + + std::cout << " release() with " << kInFlight << " in flight took " + << releaseMs << "ms -> threads " << after + << ", callbacks " << d.total.load() << "/" << kInFlight + << " worst=" << d.worst() << std::endl; + + // Fast teardown in its harshest form: 32 calls outstanding, each with 20s + // left on its clock. The old design's floor was one 25ms wait slice; there + // is no slice any more, because there is no future being polled. + EXPECT_LT(releaseMs, 100) + << "teardown is waiting for something again"; + EXPECT_EQ(d.total.load(), kInFlight); + EXPECT_EQ(d.worst(), 1); + EXPECT_EQ(d.missing(), 0); + + host.provider().letGo(); + pump(200); +} + +// ── 2. exactly once, per call, on the four ordinary outcomes ──────────────── +// +// NORMAL and DEFERRED-THEN-COMPLETED are run at volume because they are the two +// that go through the reply handler and the completion-event handler +// respectively — the two sites where the deadline is still armed and racing. +// +// These are WEAK detectors of the exactly-once gate on their own (one resolver, +// one deliver()); the two race tests further down are the strong ones. +TEST_F(IoFoldTest, NormalAndDeferredCompletionsDeliverExactlyOnceAtVolume) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + auto* plain = dynamic_cast(obj); + ASSERT_NE(plain, nullptr); + + constexpr int kNormal = 400; + constexpr int kDeferred = 400; + Deliveries normal(kNormal); + Deliveries deferred(kDeferred); + + for (int i = 0; i < kNormal; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("ping"), + QVariantList{ QVariant(i) }, 5000, + [&normal, i](QVariant v, const logos::CallError& e) { + normal.record(i, std::move(v), e); + }); + } + // Completion delays swept across the sub-millisecond band, so the event + // sometimes beats the sentinel's own reply out of the provider and + // sometimes trails it — both orders exercised rather than assumed. + for (int i = 0; i < kDeferred; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("defer"), + QVariantList{ QVariant((i % 12) * 40) }, 8000, + [&deferred, i](QVariant v, const logos::CallError& e) { + deferred.record(i, std::move(v), e); + }); + } + + pumpUntilTotal(normal, kNormal, 60000); + pumpUntilTotal(deferred, kDeferred, 60000); + pump(500); // a duplicate delivery would land here + + const Registries r = registries(plain); + std::cout << " normal " << normal.total.load() << "/" << kNormal + << " worst=" << normal.worst() << " missing=" << normal.missing() + << " code='" << normal.code() << "'" << std::endl; + std::cout << " deferred " << deferred.total.load() << "/" << kDeferred + << " worst=" << deferred.worst() << " missing=" << deferred.missing() + << " code='" << deferred.code() << "'" << std::endl; + std::cout << " after " << (kNormal + kDeferred) << " completed calls: inflight=" + << r.inflight << " deferred=" << r.deferred << " completions=" + << r.completions << " connection-pending=" << r.pendingOnConnection + << std::endl; + + EXPECT_EQ(normal.worst(), 1); + EXPECT_EQ(normal.missing(), 0); + EXPECT_TRUE(normal.code().empty()); + EXPECT_EQ(deferred.worst(), 1); + EXPECT_EQ(deferred.missing(), 0); + EXPECT_TRUE(deferred.code().empty()) << "a completed deferred call reported an error"; + { + std::lock_guard g(deferred.mu); + EXPECT_EQ(deferred.lastValue.toInt(), 7) + << "the deferred call delivered the sentinel instead of the completion"; + } + + // Retention, on both sides, after 800 completed calls on one handle. + EXPECT_EQ(r.inflight, 0u); + EXPECT_EQ(r.deferred, 0u); + EXPECT_EQ(r.completions, 0u); + EXPECT_EQ(r.pendingOnConnection, 0u); + + obj->release(); + pump(100); +} + +// TIMEOUT and CANCELLATION, the two outcomes the deadline and teardown own. Run +// in alternation so a cancellation lands while other calls are mid-timeout. +TEST_F(IoFoldTest, TimeoutAndCancellationDeliverExactlyOnce) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + constexpr int kRounds = 40; + Deliveries timedOut(kRounds); + Deliveries cancelled(kRounds); + + for (int r = 0; r < kRounds; ++r) { + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + // One that will hit its deadline, and one the release will cancel. + ch->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 250, + [&timedOut, r](QVariant v, const logos::CallError& e) { + timedOut.record(r, std::move(v), e); + }); + ch->callMethodAsyncWithError(kToken, QStringLiteral("sink"), {}, 9000, + [&cancelled, r](QVariant v, const logos::CallError& e) { + cancelled.record(r, std::move(v), e); + }); + // Let the deadline pass and the sentinel come back, so the release + // lands on a call that is genuinely parked in the deferred half. + pumpUntilTotal(timedOut, r + 1, 5000); + obj->release(); + pumpUntilTotal(cancelled, r + 1, 5000); + } + pump(500); + + std::cout << " timeout " << timedOut.total.load() << "/" << kRounds + << " worst=" << timedOut.worst() << " code='" << timedOut.code() + << "'" << std::endl; + std::cout << " cancelled " << cancelled.total.load() << "/" << kRounds + << " worst=" << cancelled.worst() << " code='" << cancelled.code() + << "'" << std::endl; + + EXPECT_EQ(timedOut.worst(), 1); + EXPECT_EQ(timedOut.missing(), 0); + EXPECT_EQ(timedOut.code(), "timeout") + << "moving the deadline onto a steady_timer must not change what it reports"; + EXPECT_EQ(cancelled.worst(), 1); + EXPECT_EQ(cancelled.missing(), 0); + EXPECT_EQ(cancelled.code(), "transport_error") + << "a call abandoned by release() is a torn-down transport, not a timeout"; + + host.provider().letGo(); + pump(200); +} + +// ── 2b. a call that hits its deadline and is THEN answered ────────────────── +// +// The shape that used to be the second resolver: the timer resolves the call, +// the reply turns up afterwards, and something has to make sure the caller is +// not told twice. TWO independent mechanisms now cover it, and this pins the +// pair: +// +// * the delivery WITHDRAWS the registration from the connection +// (cancelPending), so a reply arriving later finds no handler and is dropped +// at the transport — which is also the retention fix, and is why the count +// of pending registrations below must be zero; +// * if the reply beats the withdrawal, the exactly-once CAS in deliver() takes +// it. +// +// Be precise about what that makes this test: with an 800ms gap the withdrawal +// always wins, so it is a strong detector of the WITHDRAWAL and a weak one of +// the CAS. The strong CAS detector is ReleaseRacingAnInFlightCompletionIsSafe, +// where teardown and the completion handler genuinely arrive together. +TEST_F(IoFoldTest, ATimedOutCallThatIsLaterAnsweredStillDeliversOnce) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + auto* plain = dynamic_cast(obj); + ASSERT_NE(plain, nullptr); + + constexpr int kCalls = 24; + Deliveries d(kCalls); + for (int i = 0; i < kCalls; ++i) { + // 200ms deadline against a provider that is parked: the timer fires + // first, then letGo() releases the real reply into the same call. + ch->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 200, + [&d, i](QVariant v, const logos::CallError& e) { + d.record(i, std::move(v), e); + }); + } + pumpUntilTotal(d, kCalls, 10000); + ASSERT_EQ(d.total.load(), kCalls); + ASSERT_EQ(d.code(), "timeout"); + + host.provider().letGo(); + pump(800); // the reply arrives here, for calls already timed out + + const Registries r = registries(plain); + std::cout << " " << kCalls << " timed-out-then-answered calls -> deliveries=" + << d.total.load() << " worst=" << d.worst() + << " inflight=" << r.inflight + << " connection-pending=" << r.pendingOnConnection << std::endl; + + EXPECT_EQ(d.worst(), 1) + << "a call was delivered twice: the exactly-once gate is not holding"; + EXPECT_EQ(d.missing(), 0); + EXPECT_EQ(r.inflight, 0u); + + obj->release(); + pump(100); +} + +// ── 2c. THE STRONG exactly-once detector: teardown against replies in flight ─ +// +// Finding a race wide enough to be a reliable detector took some doing, and the +// two obvious candidates are both too narrow to trust — this comment is the map, +// because a detector that "usually" fires is not one. +// +// * timeout-then-answer (2b above) is not a race at all any more: the delivery +// withdraws the registration, so a reply arriving a comfortable interval +// later never reaches the call. +// * release-against-one-completion (further down) needs teardown to snapshot +// the call in the few instructions between the completion handler taking it +// out of `deferred` and deliver() taking it out of `inflight`. With the gate +// removed it caught nothing in 6 solo runs of 300 rounds each, and caught +// one double in a seventh run inside the full suite. It is a fine +// use-after-free hammer and an unreliable exactly-once detector; a test that +// fails one run in seven on broken code is not a gate. +// +// This one is wide by construction. Teardown snapshots the whole in-flight map +// under the lock and then delivers the calls ONE AT A TIME with the lock +// released, so with N calls outstanding the window in which the io thread can +// deliver a reply for a call teardown has already claimed is N deliveries long +// — not a handful of instructions. Both paths then arrive at deliver() for the +// same AsyncCall, and the CAS is the only thing deciding. +// +// Against a transport with the gate removed (edit (a) at the top of this file) +// this must report double deliveries. If it does not, the exactly-once +// assertions everywhere else in this file are decoration. +TEST_F(IoFoldTest, ReleaseRacingRepliesInFlightDeliversEachCallOnce) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + constexpr int kRounds = 20; + constexpr int kCalls = 500; + int doubled = 0; + int dropped = 0; + int byReply = 0; + int byTeardown = 0; + + for (int r = 0; r < kRounds; ++r) { + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + auto d = std::make_shared(kCalls); + auto codes = std::make_shared>>(2); // [reply, teardown] + for (int i = 0; i < kCalls; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("ping"), + QVariantList{ QVariant(i) }, 20000, + [d, codes, i](QVariant v, const logos::CallError& e) { + (*codes)[e.code.empty() ? 0 : 1].fetch_add(1); + d->record(i, std::move(v), e); + }); + } + // No pump: release() lands while the provider is still answering, so the + // io thread is delivering replies for exactly the calls teardown is + // walking. A tiny jittered pause sweeps where in the burst it lands. + QThread::usleep(static_cast((r % 6) * 120)); + obj->release(); + + pumpUntilTotal(*d, kCalls, 15000); + pump(50); // a duplicate would land here + + for (const auto& c : d->counts) { + if (c.load() > 1) doubled += c.load() - 1; + if (c.load() == 0) ++dropped; + } + byReply += (*codes)[0].load(); + byTeardown += (*codes)[1].load(); + } + + std::cout << " " << kRounds << " rounds x " << kCalls + << " calls released mid-burst -> answered-by-reply=" << byReply + << " cancelled-by-teardown=" << byTeardown + << " DOUBLE deliveries=" << doubled << " dropped=" << dropped + << std::endl; + + // Both resolvers have to have been live, or the race was not run. + EXPECT_GT(byReply, 0) << "no call was answered by its reply"; + EXPECT_GT(byTeardown, 0) << "no call was cancelled by teardown — release() " + "is landing after the whole burst completed and " + "this test is racing nothing"; + EXPECT_EQ(doubled, 0) << doubled << " calls were delivered more than once"; + EXPECT_EQ(dropped, 0) << dropped << " calls were never delivered at all"; + + host.provider().letGo(); + pump(200); +} + +// ── 3. the deadline is not hostage to the io thread ───────────────────────── +// +// THE ONE REGRESSION THE FIRST CUT OF THIS WORK HAD. Putting the per-call timer +// on the connection's strand looks obviously right — it serializes with the +// reply handler for free — and it makes every deadline in the process wait on a +// single thread that ordinary module code is allowed to occupy: this transport +// runs user onEvent callbacks INLINE on it (rpc_connection.h dispatchIncoming), +// and an event handler calling another module is not exotic. +// +// This is the exact shape that failed, and the numbers it produced: an onEvent +// handler holding the io thread for 2000ms, while a 200ms deadline is +// outstanding on a COMPLETELY DIFFERENT connection. On the pre-fold design that +// deadline fires at ~200ms because it has its own thread. With the timer on the +// shared strand it fired at 2003ms. It must be back to ~200ms. +TEST_F(IoFoldTest, DeadlineFiresOnTimeWhileTheIoThreadIsBusy) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + + // TWO connections, to make the point that this is not about one call + // queueing behind another on the same socket: they share nothing except the + // process-wide io_context, which is the whole problem. + auto connA = connectTo(host.port()); + ASSERT_NE(connA, nullptr); + auto connB = connectTo(host.port()); + ASSERT_NE(connB, nullptr); + + LogosObject* a = connA->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(a, nullptr); + auto* chA = channelFor(a); + ASSERT_NE(chA, nullptr); + LogosObject* b = connB->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(b, nullptr); + auto* chB = channelFor(b); + ASSERT_NE(chB, nullptr); + + std::atomic handlerRunning{false}; + std::atomic handlerDone{false}; + std::atomic handlerHeldMs{0}; + const std::thread::id mainThread = std::this_thread::get_id(); + std::atomic onIoThread{false}; + + a->onEvent(QStringLiteral("tick"), [&](const QString&, const QVariantList&) { + onIoThread.store(std::this_thread::get_id() != mainThread); + handlerRunning.store(true); + QElapsedTimer held; + held.start(); + // A SYNCHRONOUS call with a 2000ms budget, from inside an event handler. + // Ordinary module code. It cannot be answered — the thread that would + // decode the reply is this one — so it occupies the io thread for its + // full 2000ms and then reports a timeout, which is precisely the + // "handler that takes a while" case. + logos::CallError err; + chA->callMethodWithError(kToken, QStringLiteral("block"), {}, 2000, &err); + handlerHeldMs.store(held.elapsed()); + handlerDone.store(true); + }); + + // Kick the event off. `fire` makes the provider emit "tick" back at us. + Deliveries fired(1); + chA->callMethodAsyncWithError(kToken, QStringLiteral("fire"), {}, 5000, + [&fired](QVariant v, const logos::CallError& e) { + fired.record(0, std::move(v), e); + }); + + // Wait for the handler to actually be on the io thread and blocking. + { + QElapsedTimer t; + t.start(); + while (!handlerRunning.load() && t.elapsed() < 5000) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); + } + ASSERT_TRUE(handlerRunning.load()) << "the event handler never ran"; + ASSERT_TRUE(onIoThread.load()) + << "the event did not arrive on the io thread — this test is not " + "exercising the coupling it claims to"; + + // Now, with the io thread held, put a 200ms deadline on the OTHER connection. + Deliveries d(1); + QElapsedTimer deadline; + deadline.start(); + chB->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 200, + [&d](QVariant v, const logos::CallError& e) { + d.record(0, std::move(v), e); + }); + pumpUntilTotal(d, 1, 6000); + const qint64 firedAt = deadline.elapsed(); + // Sampled BEFORE the unwind below, because it is half the claim: the io + // thread has to still be held at the moment the deadline fires, or this + // test is measuring an idle process. + const bool stillHeld = !handlerDone.load(); + + // Let everything unwind, so the held time can be reported rather than + // guessed at. + host.provider().letGo(); + { + QElapsedTimer t; + t.start(); + while (!handlerDone.load() && t.elapsed() < 8000) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); + } + + std::cout << " io thread held by an onEvent handler for " + << handlerHeldMs.load() << "ms (still held when the deadline fired: " + << stillHeld << "); a 200ms deadline on another connection fired at " + << firedAt << "ms" << std::endl; + + EXPECT_EQ(d.total.load(), 1) << "the deadline never fired at all"; + EXPECT_EQ(d.code(), "timeout"); + EXPECT_LT(firedAt, 800) + << "the deadline waited for the io thread: fired at " << firedAt + << "ms instead of ~200ms. The per-call timer is coupled to the shared " + "io_context again."; + EXPECT_GE(firedAt, 150) + << "the deadline fired early — this is measuring something else"; + EXPECT_TRUE(stillHeld) + << "the handler let go before the deadline fired; the test proved nothing"; + + pump(300); + a->release(); + b->release(); + pump(100); +} + +// The harsher half: a handler that never lets go at all. With the deadline on +// the shared io thread this call would hang for as long as the handler does, +// which is forever — the timeout stops existing exactly when it is most needed. +TEST_F(IoFoldTest, DeadlineFiresWhileTheIoThreadIsBlockedForever) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto connA = connectTo(host.port()); + ASSERT_NE(connA, nullptr); + auto connB = connectTo(host.port()); + ASSERT_NE(connB, nullptr); + + LogosObject* a = connA->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(a, nullptr); + auto* chA = channelFor(a); + ASSERT_NE(chA, nullptr); + LogosObject* b = connB->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(b, nullptr); + auto* chB = channelFor(b); + ASSERT_NE(chB, nullptr); + + std::mutex mu; + std::condition_variable cv; + bool letHandlerGo = false; + std::atomic handlerRunning{false}; + std::atomic handlerDone{false}; + + // The handler holds the process's only io thread, so EVERY exit from this + // function — including a failed ASSERT — has to let it go. Without this a + // regression here does not fail the suite, it hangs it, and every test that + // runs afterwards hangs too. + struct Unblock { + std::mutex* mu; std::condition_variable* cv; bool* flag; + ~Unblock() + { + { std::lock_guard g(*mu); *flag = true; } + cv->notify_all(); + } + } unblock{&mu, &cv, &letHandlerGo}; + + a->onEvent(QStringLiteral("tick"), [&](const QString&, const QVariantList&) { + handlerRunning.store(true); + std::unique_lock lk(mu); + cv.wait(lk, [&] { return letHandlerGo; }); + handlerDone.store(true); + }); + + Deliveries fired(1); + chA->callMethodAsyncWithError(kToken, QStringLiteral("fire"), {}, 5000, + [&fired](QVariant v, const logos::CallError& e) { + fired.record(0, std::move(v), e); + }); + { + QElapsedTimer t; + t.start(); + while (!handlerRunning.load() && t.elapsed() < 5000) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); + } + ASSERT_TRUE(handlerRunning.load()) << "the event handler never ran"; + + Deliveries d(1); + QElapsedTimer deadline; + deadline.start(); + chB->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 250, + [&d](QVariant v, const logos::CallError& e) { + d.record(0, std::move(v), e); + }); + pumpUntilTotal(d, 1, 4000); + const qint64 firedAt = deadline.elapsed(); + + std::cout << " io thread blocked with no end in sight; a 250ms deadline " + << (d.total.load() ? "fired at " : "NEVER FIRED (") + << firedAt << "ms" << (d.total.load() ? "" : ")") + << ", handler still blocked: " << (!handlerDone.load()) << std::endl; + + EXPECT_FALSE(handlerDone.load()) + << "the handler unblocked itself; the test proved nothing"; + EXPECT_EQ(d.total.load(), 1) + << "the deadline never fired: it is waiting for an io thread that is " + "never coming back"; + if (d.total.load() == 1) { + EXPECT_EQ(d.code(), "timeout"); + EXPECT_LT(firedAt, 1500); + } + + { + std::lock_guard g(mu); + letHandlerGo = true; + } + cv.notify_all(); + host.provider().letGo(); + { + QElapsedTimer t; + t.start(); + while (!handlerDone.load() && t.elapsed() < 8000) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); + } + pump(300); + a->release(); + b->release(); + pump(100); +} + +// The same deadline, with nothing in the way: the accuracy the timer thread +// delivers when the process is idle. This is the baseline the two tests above +// are compared against, and it is what a caller's timeoutMs actually means. +TEST_F(IoFoldTest, DeadlineAccuracyWhenIdle) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + constexpr int kRounds = 12; + constexpr int kDeadlineMs = 200; + std::vector observed; + for (int i = 0; i < kRounds; ++i) { + Deliveries d(1); + QElapsedTimer t; + t.start(); + ch->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, kDeadlineMs, + [&d](QVariant v, const logos::CallError& e) { + d.record(0, std::move(v), e); + }); + pumpUntilTotal(d, 1, 5000); + ASSERT_EQ(d.total.load(), 1); + ASSERT_EQ(d.code(), "timeout"); + observed.push_back(t.elapsed()); + } + std::sort(observed.begin(), observed.end()); + const qint64 lo = observed.front(); + const qint64 med = observed[observed.size() / 2]; + const qint64 hi = observed.back(); + std::cout << " " << kRounds << " idle " << kDeadlineMs + << "ms deadlines: min=" << lo << " median=" << med + << " max=" << hi << "ms" << std::endl; + + EXPECT_GE(lo, kDeadlineMs - 20) << "a deadline fired early"; + // Generous, because the delivery hop through the Qt event loop and this + // test's own 5ms pump granularity are both inside the measurement. + EXPECT_LE(med, kDeadlineMs + 120); + EXPECT_LE(hi, kDeadlineMs + 400); + + host.provider().letGo(); + obj->release(); + pump(100); +} + +// ── 4. retention: what a call that is never answered leaves behind ────────── +// +// TWO registries, and until this change only one of them was emptied. The +// handle's CallState::inflight is erased by the delivery. The CONNECTION's +// m_pendingCalls was erased by exactly two events — a decoded reply carrying +// that id, and fail()'s teardown sweep — so a call resolved by its DEADLINE was +// in neither, and its registration stayed for the life of the connection, which +// outlives every handle it hands out. That was true of the promise it held +// before the fold too; the fold made the orphan bigger (a handler closing over +// the caller's std::function rather than a promise), so it is closed here +// rather than inherited: AsyncCall::deliver() withdraws the registration. +TEST_F(IoFoldTest, CallsResolvedByTheirDeadlineLeaveNothingBehind) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + auto* plain = dynamic_cast(obj); + ASSERT_NE(plain, nullptr); + + // Every one of these times out against a provider parked in `block`, and is + // never answered — the exact shape nothing used to clean up. + constexpr int kCalls = 200; + Deliveries d(kCalls); + for (int i = 0; i < kCalls; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("block"), {}, 60, + [&d, i](QVariant v, const logos::CallError& e) { + d.record(i, std::move(v), e); + }); + } + pumpUntilTotal(d, kCalls, 30000); + pump(300); + + const Registries r = registries(plain); + std::cout << " " << kCalls << " deadline-orphaned calls -> inflight=" + << r.inflight << " deferred=" << r.deferred + << " connection-pending=" << r.pendingOnConnection + << " deliveries=" << d.total.load() << " worst=" << d.worst() + << " code='" << d.code() << "'" << std::endl; + + EXPECT_EQ(d.total.load(), kCalls); + EXPECT_EQ(d.worst(), 1); + EXPECT_EQ(d.code(), "timeout"); + EXPECT_EQ(r.inflight, 0u); + EXPECT_EQ(r.deferred, 0u); + EXPECT_EQ(r.pendingOnConnection, 0u) + << r.pendingOnConnection << " of " << kCalls << " calls left their " + << "registration in the connection's pending map: retention is growing " + << "with call count on the connection, which outlives every handle."; + + host.provider().letGo(); + pump(200); + obj->release(); + pump(100); +} + +// A "multi" provider can answer the pending sentinel AFTER the caller's deadline +// has passed. The deadline has already resolved the call by then; the reply +// handler then arrives, sees a sentinel, and — in the first cut of this design — +// filed the AsyncCall under CallState::deferred and re-armed. Nothing took it out +// again, because the re-armed deadline's deliver() returned at the exactly-once +// gate before reaching the erase: one leaked map entry per slow-sentinel call, +// which is exactly the retention the fold exists to remove, reintroduced by a +// different route. +// +// The fix is two-part and both parts are load-bearing: deliver() leaves the +// registries BEFORE the gate rather than after it, and the reply handler declines +// to file a call that is already delivered. +TEST_F(IoFoldTest, ASentinelArrivingAfterItsDeadlineDoesNotLeakARegistryEntry) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + auto* plain = dynamic_cast(obj); + ASSERT_NE(plain, nullptr); + + // The provider sleeps 400ms per call on its single proxy thread, so these + // serialize; each one's 100ms deadline is long gone when its sentinel lands. + constexpr int kCalls = 6; + Deliveries d(kCalls); + for (int i = 0; i < kCalls; ++i) { + ch->callMethodAsyncWithError(kToken, QStringLiteral("slowsink"), {}, 100, + [&d, i](QVariant v, const logos::CallError& e) { + d.record(i, std::move(v), e); + }); + } + pumpUntilTotal(d, kCalls, 20000); + // Every call has timed out; now let the late sentinels arrive and be + // processed. This is the window the leak opens in. + pump(3000); + + const Registries r = registries(plain); + std::cout << " " << kCalls << " late-sentinel calls -> inflight=" << r.inflight + << " deferred=" << r.deferred << " connection-pending=" + << r.pendingOnConnection << " deliveries=" << d.total.load() + << " worst=" << d.worst() << " code='" << d.code() << "'" << std::endl; + + EXPECT_EQ(d.total.load(), kCalls); + EXPECT_EQ(d.worst(), 1); + EXPECT_EQ(d.code(), "timeout"); + EXPECT_EQ(r.inflight, 0u); + EXPECT_EQ(r.deferred, 0u) + << "a sentinel that arrived after its own deadline left " << r.deferred + << " entries behind: retention is growing with call count again"; + EXPECT_EQ(r.pendingOnConnection, 0u); + + obj->release(); + pump(100); +} + +// ── 5. release racing an in-flight COMPLETION ─────────────────────────────── +// +// The hazard that already existed, and the second strong exactly-once detector. +// The completion-event subscription runs on the Asio io thread, RpcConnection +// invokes it with its own lock released, and NOTHING joins that thread. Under +// the thread-per-call design the subscription captured raw `this`, so release()'s +// `delete this` could land inside the callback — reproduced as a SIGSEGV under +// Guard Malloc in test_plain_completion_sub_lifetime.cpp, which is a separate +// change from this one and where that detector lives. +// +// What this adds is the FOLD's version of the same race: teardown and the +// completion handler both trying to resolve the same call. It is one of the two +// places where two resolvers genuinely arrive at deliver() for one call, so it +// is also where a broken exactly-once gate shows up. +// +// Run it under Guard Malloc to make a freed access fatal rather than +// probabilistic: +// DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib \ +// ./protocol_tests --gtest_filter='IoFoldTest.ReleaseRacing*' +TEST_F(IoFoldTest, ReleaseRacingAnInFlightCompletionIsSafe) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + constexpr int kRounds = 300; + int done = 0; + int doubled = 0; + QElapsedTimer total; + total.start(); + + for (int i = 0; i < kRounds; ++i) { + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + auto d = std::make_shared(1); + // 0..~2.4ms of completion delay, swept, so the release below lands + // before, during and after the completion callback across the run. + ch->callMethodAsyncWithError(kToken, QStringLiteral("defer"), + QVariantList{ QVariant((i % 25) * 100) }, 8000, + [d](QVariant v, const logos::CallError& e) { + d->record(0, std::move(v), e); + }); + if (i % 4 != 0) + QThread::usleep(static_cast((i % 25) * 100)); + + obj->release(); + + // Whatever the race decided, the caller is told once: either the + // completion landed first (value 7) or the release did (transport_error). + pumpUntilTotal(*d, 1, 5000); + pump(5); + if (d->total.load() > 1) ++doubled; + ASSERT_EQ(d->total.load(), 1) + << "round " << i << ": " << d->total.load() << " callbacks, not one"; + ++done; + } + + std::cout << " " << done << "/" << kRounds + << " release-during-completion rounds, exactly one callback each, in " + << total.elapsed() << "ms (double deliveries: " << doubled << ")" + << std::endl; + EXPECT_EQ(done, kRounds); + EXPECT_EQ(doubled, 0); +} + +// ── why "wait for the io thread" was not an option ────────────────────────── +// +// The obvious alternative to shared ownership is a barrier: post a no-op onto +// the strand at teardown and block until it runs, which would prove no handler +// is mid-flight. It cannot be used here, and this is the reason. +// +// IoContextPool runs EXACTLY ONE worker thread and is a process-wide singleton +// (io_context_pool.cpp), and the plain transport delivers user event callbacks +// INLINE on it (rpc_connection.h). So a user handler that releases its handle — +// the reentrant-release-from-event-dispatch shape remote_transport.cpp documents +// as shipped production behaviour — is running ON the only thread that could +// ever drain that barrier. It would wedge the process. (Measured: it does.) +// +// This design has nothing to wait for, so the same call just returns. The +// watchdog turns a regression into a named abort rather than a CI job that hangs +// until its timeout. +TEST_F(IoFoldTest, ReleaseFromInsideAnIoThreadEventCallbackDoesNotWedge) +{ + LiveHost host; + ASSERT_TRUE(host.ok()); + auto conn = connectTo(host.port()); + ASSERT_NE(conn, nullptr); + + LogosObject* obj = conn->requestObject(QStringLiteral("omni_module"), 5000); + ASSERT_NE(obj, nullptr); + auto* ch = channelFor(obj); + ASSERT_NE(ch, nullptr); + + std::atomic releasedFromCallback{false}; + std::atomic onIoThread{false}; + const std::thread::id mainThread = std::this_thread::get_id(); + + obj->onEvent(QStringLiteral("tick"), + [&](const QString&, const QVariantList&) { + onIoThread.store(std::this_thread::get_id() != mainThread); + obj->release(); // reentrant, on the io thread + releasedFromCallback.store(true); + }); + + // A call left outstanding, so the reentrant release has real work to do: + // it must cancel this and deliver its callback. + Deliveries d(1); + ch->callMethodAsyncWithError(kToken, QStringLiteral("sink"), {}, 9000, + [&d](QVariant v, const logos::CallError& e) { + d.record(0, std::move(v), e); + }); + pump(200); + ASSERT_EQ(d.total.load(), 0); + + // A watchdog, because the failure mode is a hang and not an assertion. + std::atomic finished{false}; + std::thread dog([&] { + for (int i = 0; i < 200 && !finished.load(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + if (!finished.load()) { + std::fprintf(stderr, "\nWATCHDOG: reentrant release() from an io-thread " + "event callback wedged the process.\n"); + std::fflush(stderr); + std::abort(); + } + }); + + // Fire the event. The provider's own reply to `fire` is irrelevant; what + // matters is that the event handler runs on the io thread and releases. + auto fired = std::make_shared(1); + ch->callMethodAsyncWithError(kToken, QStringLiteral("fire"), {}, 5000, + [fired](QVariant v, const logos::CallError& e) { + fired->record(0, std::move(v), e); + }); + + QElapsedTimer t; + t.start(); + while (!releasedFromCallback.load() && t.elapsed() < 5000) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); + finished.store(true); + dog.join(); + + pumpUntilTotal(d, 1, 3000); + pump(300); + + std::cout << " reentrant release from the io thread returned after " + << t.elapsed() << "ms (on io thread: " << onIoThread.load() + << "), abandoned call delivered " << d.total.load() + << " time(s) code='" << d.code() << "'" << std::endl; + + EXPECT_TRUE(releasedFromCallback.load()) + << "release() never returned from inside the io-thread event callback"; + EXPECT_TRUE(onIoThread.load()) + << "the event did not arrive on the io thread — this test is not " + "exercising the reentrancy it claims to"; + EXPECT_EQ(d.total.load(), 1); + EXPECT_EQ(d.code(), "transport_error"); + + host.provider().letGo(); + pump(200); +} diff --git a/tests/protocol/test_plain_cancel_pending_race.cpp b/tests/protocol/test_plain_cancel_pending_race.cpp new file mode 100644 index 0000000..4e0393d --- /dev/null +++ b/tests/protocol/test_plain_cancel_pending_race.cpp @@ -0,0 +1,410 @@ +// WHERE EXACTLY-ONCE RESTS, ONCE cancelPending() EXISTS. +// +// cancelPending() withdraws a call's registration from the connection's pending +// map. It is tempting — and rpc_connection.h used to say — that a caller which +// has given up therefore "is never called back at all". It is not true, and the +// distance between the two matters, because it decides whether the callers of +// sendCallAsync() need idempotent handlers or merely tidy ones. +// +// dispatchIncoming() COPIES the handler out of the map under m_mu and invokes it +// with the mutex RELEASED. A cancelPending() that arrives in that gap erases an +// entry that is no longer there and returns having stopped nothing; the handler +// then runs to completion. So: +// +// * at-most-once INVOCATION of a registered handler is the connection's, and +// it comes from the extract-and-erase under m_mu — three contenders +// (dispatchIncoming, fail()'s sweep, cancelPending) and only one can win; +// * exactly-once DELIVERY to the user is NOT the connection's. It belongs to +// the handler, and for PlainLogosObject it is AsyncCall::deliver()'s CAS. +// +// These tests construct that interleaving by hand rather than racing for it: a +// connection stub reproduces dispatchIncoming's extract-then-invoke exactly and +// lets the test stand between the two halves. +// +// The second test is a validated detector: against a transport whose +// AsyncCall::claim() does not compare-exchange and whose takeCallback() copies +// rather than swaps — a local edit in a throwaway checkout, not a switch in +// this tree; see the top of test_iofold.cpp — it reports 2 deliveries for 1 +// call. + +#include + +#include "incoming_call_handler.h" +#include "json_codec.h" +#include "logos_call_error.h" +#include "plain_logos_object.h" +#include "rpc_connection.h" +#include "rpc_message.h" + +#include +#include +#include +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace logos::plain; + +namespace { + +// A connection that goes no further than its pending map, so a test can occupy +// the gap dispatchIncoming leaves between taking a handler out and running it. +// Everything below m_pending mirrors RpcConnection: registration under the +// mutex, extract-and-erase under the mutex, invocation with it released. +class StubConnection : public RpcConnectionBase { +public: + void start() override {} + void stop(const std::string&) override { m_open = false; } + bool isOpen() const override { return m_open; } + + std::future sendCall(CallMessage msg) override + { + // The OTHER shape of handler in this codebase: a promise. Registered + // through the same path, deliberately, so the tests below can reach it. + auto p = std::make_shared>(); + auto f = p->get_future(); + sendCallAsync(std::move(msg), [p](ResultMessage r) { + try { p->set_value(std::move(r)); } catch (...) {} + }); + return f; + } + + void sendCallAsync(CallMessage msg, ResultHandler handler) override + { + if (!handler) return; + std::lock_guard g(m_mu); + m_lastId = msg.id; + m_pending[msg.id] = std::move(handler); + } + + std::future sendMethods(MethodsMessage msg) override + { + std::promise p; + MethodsResultMessage r; r.id = msg.id; r.ok = true; + p.set_value(std::move(r)); + return p.get_future(); + } + + void cancelPending(std::uint64_t id) override + { + std::lock_guard g(m_mu); + m_pending.erase(id); + m_cancels.fetch_add(1); + } + + void sendSubscribe(SubscribeMessage, std::function) override {} + void sendUnsubscribe(UnsubscribeMessage) override {} + void sendEvent(EventMessage) override {} + void sendToken(TokenMessage) override {} + void setErrorHandler(ErrorHandler) override {} + std::uint64_t nextId() override { return m_next.fetch_add(1); } + + // dispatchIncoming's FIRST half, verbatim: find under the mutex, move out, + // erase, drop the mutex. The caller decides when the second half runs. + ResultHandler extract(std::uint64_t id) + { + std::lock_guard g(m_mu); + auto it = m_pending.find(id); + if (it == m_pending.end()) return nullptr; + ResultHandler h = std::move(it->second); + m_pending.erase(it); + return h; + } + + std::uint64_t lastId() const { return m_lastId; } + int cancels() const { return m_cancels.load(); } + size_t pendingCount() { std::lock_guard g(m_mu); return m_pending.size(); } + +private: + std::mutex m_mu; + std::map m_pending; + std::atomic m_next{1}; + std::atomic m_lastId{0}; + std::atomic m_cancels{0}; + bool m_open = true; +}; + +QCoreApplication* ensureApp() +{ + static int argc = 0; + static char* argv[] = { nullptr }; + if (!QCoreApplication::instance()) new QCoreApplication(argc, argv); + return QCoreApplication::instance(); +} + +// Deliveries land on the Qt loop, so nothing is counted until it is pumped. +void pumpUntil(std::atomic& counter, int target, int budgetMs) +{ + QElapsedTimer t; t.start(); + while (counter.load() < target && t.elapsed() < budgetMs) + QCoreApplication::processEvents(QEventLoop::AllEvents, 5); +} + +void pump(int ms) +{ + QElapsedTimer t; t.start(); + while (t.elapsed() < ms) QCoreApplication::processEvents(QEventLoop::AllEvents, 5); +} + +ResultMessage okResult(std::uint64_t id, int value) +{ + ResultMessage res; + res.id = id; + res.ok = true; + res.value = RpcValue{static_cast(value)}; + return res; +} + +} // namespace + +// ── 1. the literal claim: cancelPending() does NOT prevent a callback ──────── +TEST(PlainCancelPendingRaceTest, CancelPendingCannotStopAnAlreadyExtractedHandler) +{ + ensureApp(); + auto conn = std::make_shared(); + auto* obj = new PlainLogosObject("cancel_probe", conn); + + std::atomic deliveries{0}; + obj->callMethodAsyncWithError(QStringLiteral("tok"), QStringLiteral("ping"), + QVariantList{}, 20000, + [&deliveries](QVariant, const logos::CallError&) { + deliveries.fetch_add(1); + }); + const std::uint64_t id = conn->lastId(); + + // The reply is decoded: the handler leaves the map. + RpcConnectionBase::ResultHandler h = conn->extract(id); + ASSERT_TRUE(h) << "the call never registered a handler"; + + // The caller gives up HERE, in the gap. cancelPending() finds nothing. + conn->cancelPending(id); + EXPECT_EQ(conn->pendingCount(), 0u); + EXPECT_EQ(deliveries.load(), 0) << "nothing should have been delivered yet"; + + // ...and the handler runs anyway. + h(okResult(id, 42)); + pumpUntil(deliveries, 1, 2000); + + EXPECT_EQ(deliveries.load(), 1) + << "cancelPending() ran before this callback and did not stop it — which " + "is the point: the comment claiming a cancelled caller 'is never " + "called back at all' is what is wrong, not the code"; + + obj->release(); + pump(50); +} + +// ── 2. so exactly-once has to come from somewhere else: the CAS ───────────── +// +// The same gap, but with the second resolver being the one that actually exists +// in production — teardown, which cancels every outstanding call. Both paths +// reach AsyncCall::deliver() for the same call. With the CAS removed this +// reports 2. +TEST(PlainCancelPendingRaceTest, AnExtractedReplyRacingTeardownDeliversExactlyOnce) +{ + ensureApp(); + auto conn = std::make_shared(); + auto* obj = new PlainLogosObject("cancel_probe", conn); + + std::atomic deliveries{0}; + std::atomic released{0}; // callErrorReleased — teardown won + std::atomic answered{0}; // no error — the reply won + obj->callMethodAsyncWithError(QStringLiteral("tok"), QStringLiteral("ping"), + QVariantList{}, 20000, + [&](QVariant, const logos::CallError& e) { + deliveries.fetch_add(1); + if (e.ok()) answered.fetch_add(1); + else released.fetch_add(1); + }); + const std::uint64_t id = conn->lastId(); + + // The io thread has the handler in hand... + RpcConnectionBase::ResultHandler h = conn->extract(id); + ASSERT_TRUE(h); + + // ...teardown resolves the call and frees the handle out from under it... + obj->release(); + + // ...and only then does the reply run. It holds a shared_ptr to its + // AsyncCall, so it is safe to run at all — and finds the gate taken. + h(okResult(id, 42)); + + pumpUntil(deliveries, 1, 2000); + pump(100); // a second delivery would land in here + + EXPECT_EQ(deliveries.load(), 1) + << "the call was delivered " << deliveries.load() + << " times; exactly-once is AsyncCall::deliver()'s CAS and nothing else"; + EXPECT_EQ(released.load(), 1) << "teardown should have been the resolver"; + EXPECT_EQ(answered.load(), 0); + + pump(50); +} + +// ── 3. the OTHER caller of sendCallAsync: a promise, not an AsyncCall ──────── +// +// RpcConnection::sendCall() registers a promise-fulfilling handler through the +// same path, and callMethodWithError() / getMethods() call cancelPending() on +// their timeout. Neither of those handlers has a CAS — so the question is +// whether at-most-once INVOCATION is enough for them, and it is: the extract is +// what makes it single, and firing into a future nobody will read is harmless. +// Run here rather than argued, on a REAL RpcConnection. +TEST(PlainCancelPendingRaceTest, ThePromiseShapedHandlerSurvivesTheSameGap) +{ + auto conn = std::make_shared(); + + CallMessage msg; + msg.id = conn->nextId(); + msg.object = "promise_probe"; + msg.method = "ping"; + auto fut = conn->sendCall(std::move(msg)); + const std::uint64_t id = conn->lastId(); + + // Extract, cancel in the gap, then run the promise handler. + RpcConnectionBase::ResultHandler h = conn->extract(id); + ASSERT_TRUE(h); + conn->cancelPending(id); + EXPECT_EQ(conn->pendingCount(), 0u) + << "nobody else can reach this handler now — that is what makes the " + "single invocation single"; + + // Fulfils a future the caller has already walked away from. No throw, no + // second invocation possible. + EXPECT_NO_THROW(h(okResult(id, 7))); + ASSERT_EQ(fut.wait_for(std::chrono::seconds(1)), std::future_status::ready); + EXPECT_TRUE(fut.get().ok); + + // And the abandoned-future case, which is what the sync path actually does: + // the caller times out, returns, and its future dies before the reply lands. + CallMessage msg2; + msg2.id = conn->nextId(); + msg2.object = "promise_probe"; + msg2.method = "ping"; + { + auto doomed = conn->sendCall(std::move(msg2)); + (void)doomed; // goes out of scope exactly as callMethodWithError's does + } + const std::uint64_t id2 = conn->lastId(); + RpcConnectionBase::ResultHandler h2 = conn->extract(id2); + ASSERT_TRUE(h2); + conn->cancelPending(id2); + EXPECT_NO_THROW(h2(okResult(id2, 9))) + << "setting a value on a shared state whose future is gone must be a " + "no-op, not a throw"; +} + +// ── 4. the same gap on the REAL connection, not a stub ─────────────────────── +// +// Everything above uses a stub that copies dispatchIncoming's shape. This checks +// the shape really is the connection's: a genuine RpcConnection pair, a provider +// that HOLDS its reply until the test says so, and a caller that cancels in +// between. The reply then travels the real wire into the real dispatchIncoming, +// which must find no registration and drop it in silence. +TEST(PlainCancelPendingRaceTest, ARealConnectionDropsAReplyThatArrivesAfterCancel) +{ + using LocalSocket = boost::asio::local::stream_protocol::socket; + using LocalConnection = RpcConnection; + + // A provider that answers nothing until told to. + class HeldReplyProvider : public IncomingCallHandler { + public: + void onCall(const CallMessage& req, CallReply reply) override + { + std::lock_guard g(mu); + id = req.id; + held = std::move(reply); + arrived.fetch_add(1); + } + void onMethods(const MethodsMessage& req, MethodsReply reply) override + { + MethodsResultMessage r; r.id = req.id; r.ok = true; reply(std::move(r)); + } + void onSubscribe(const SubscribeMessage&, EventSink, const void*) override {} + void onUnsubscribe(const UnsubscribeMessage&, const void*) override {} + void onConnectionClosed(const void*) override {} + void onToken(const TokenMessage&) override {} + + void answer() + { + CallReply r; + std::uint64_t rid = 0; + { + std::lock_guard g(mu); + r = std::move(held); + rid = id; + } + if (!r) return; + ResultMessage res; + res.id = rid; res.ok = true; + res.value = RpcValue{static_cast(1)}; + r(std::move(res)); + } + + std::mutex mu; + CallReply held; + std::uint64_t id = 0; + std::atomic arrived{0}; + }; + + boost::asio::io_context ioc; + auto guard = boost::asio::make_work_guard(ioc); + std::thread worker([&ioc] { ioc.run(); }); + + LocalSocket a(ioc), b(ioc); + boost::system::error_code ec; + boost::asio::local::connect_pair(a, b, ec); + ASSERT_FALSE(ec) << ec.message(); + + HeldReplyProvider provider; + auto codec = std::make_shared(); + auto client = std::make_shared(std::move(a), codec, nullptr); + auto provider_conn = + std::make_shared(std::move(b), codec, &provider); + client->start(); + provider_conn->start(); + + std::atomic handlerRuns{0}; + CallMessage msg; + msg.id = client->nextId(); + msg.object = "real_probe"; + msg.method = "ping"; + const std::uint64_t id = msg.id; + client->sendCallAsync(std::move(msg), [&handlerRuns](ResultMessage) { + handlerRuns.fetch_add(1); + }); + + // Wait for the provider to be holding the call, then give up on it. + QElapsedTimer t; t.start(); + while (provider.arrived.load() == 0 && t.elapsed() < 5000) + std::this_thread::sleep_for(std::chrono::milliseconds(2)); + ASSERT_EQ(provider.arrived.load(), 1) << "the call never reached the provider"; + + client->cancelPending(id); + + // Only now does the answer go out, over the real wire. + provider.answer(); + std::this_thread::sleep_for(std::chrono::milliseconds(300)); + + EXPECT_EQ(handlerRuns.load(), 0) + << "a reply arriving after cancelPending() must find no registration — " + "that is the semantic cancelPending() exists for, and it is the half " + "of the comment that IS true"; + + client->stop(); + provider_conn->stop(); + guard.reset(); + ioc.stop(); + worker.join(); +} diff --git a/tests/protocol/test_plain_waiter_publish_is_last.cpp b/tests/protocol/test_plain_waiter_publish_is_last.cpp deleted file mode 100644 index 75dbd95..0000000 --- a/tests/protocol/test_plain_waiter_publish_is_last.cpp +++ /dev/null @@ -1,1072 +0,0 @@ -// PUBLISHING IS A WAITER'S LAST ACCESS TO THE OBJECT. -// -// Its two sibling suites pin the parts of the waiter mechanism that can be -// observed by watching what the object DOES: test_plain_object_teardown.cpp -// pins what happens to a call still in flight when the handle goes away, and -// test_plain_waiter_reaping.cpp pins that finished waiters are retired and that -// retiring them cannot deadlock. This one pins the ORDERING RULE those two rest -// on, which nothing they do can see. -// -// The rule, from plain_logos_object.cpp. A waiter's exit guard runs -// reapFinishedWaiters() and then publishFinishedWaiter(id), and the publish is -// strictly its last access to the object: -// -// struct FinishOnExit { -// ~FinishOnExit() { -// self->reapFinishedWaiters(); // others, never itself -// self->publishFinishedWaiter(id); // strictly last -// } -// }; -// -// Why that is load-bearing. reapFinishedWaiters() takes the ids waiters have -// published, ERASES those entries from m_waiters under m_waiterMu, and joins the -// threads OUTSIDE the lock. stopAndJoinWaiters() (release() / the destructor) -// swaps m_waiters under the same lock and brute-force joins whatever it got. So -// a waiter that a concurrent reaper is mid-join on is NOT in teardown's map, and -// teardown can return — with release() going straight on to `delete this` — -// while that waiter is still unwinding. stopAndJoinWaiters() says so in its own -// comment: the guarantee is not "everything is joined when this returns" but -// "no waiter touches this object after this returns". Publishing being last is -// the entire reason the second sentence is true. -// -// WHAT THIS IS AND IS NOT. It is NOT a lurking use-after-free that this file -// exposes. Be precise, because the window above was overstated in review: on -// every version of this code, a supported caller's waiters are all joined -// transitively, so "teardown returns while a reaped waiter is still unwinding" -// is not reachable. A waiter leaves m_waiters exactly two ways, and both are -// covered: -// -// * teardown takes it — and teardown joins it; -// * a reaper takes it — and that reaper is either ANOTHER WAITER, which is -// itself still registered (it reaps before it publishes), so teardown joins -// the reaper and therefore waits out the join it is in; or the ASYNC-SPAWN -// path, whose join completes before callMethodAsyncWithError returns. -// -// The one reaper nobody waits for is the spawn path racing a concurrent -// release() — i.e. a caller still issuing calls on an object another thread is -// destroying, which is caller-side UB on any version of this class and faults -// on correct code too. -// -// So publish-is-last is an invariant the DESIGN RESTS ON and documents, not a -// hole. This suite pins it against future edits — the TODO above the waiter -// (fold the wait into the shared Asio io_context) moves where reaping happens, -// which is exactly the kind of change that could add an access below the -// publish. It does not close an open bug, and nothing here should be read as -// reporting one. -// -// WHY THAT NEEDS ITS OWN MECHANISM. Because "no supported caller can provoke -// it" also means no test can provoke it, so the defect ships green: with a -// touch added after the publish, the whole of PlainObjectTeardownTest and -// PlainWaiterReapingTest passes cleanly, including under Guard Malloc. A test -// built on the unsupported race would be red on correct code, which is not a -// detector. -// -// So this suite does not race anything. It OBSERVES the accesses directly, in -// two halves, because the object splits cleanly into the state a waiter must -// never touch and the registry it exists to update. -// -// HALF ONE — the state (WaiterTouchesNothingOnTheStatePageAfterPublishing). -// -// * The object is placement-newed into an mmap'd two-page arena, positioned so -// that a page boundary falls at (or just under) m_waiterMu — i.e. the members -// teardown coordinates on (m_waiterMu, m_waiters, m_finishedWaiters, -// m_nextWaiterId, m_stopping) land on the second page and everything else -// (m_objectName, m_conn, m_mu, m_subs, the completion rendezvous) on the -// first. -// * The first page is mprotect(PROT_NONE)'d for exactly as long as a waiter is -// running, and a SIGSEGV/SIGBUS handler RECORDS each access — faulting -// address, thread, and how many ids had been published at that instant — then -// unprotects so the access proceeds. Nothing crashes; the access is -// evidence, not a punishment. The handler cannot re-arm the page itself (the -// faulting instruction would fault forever), so the OBSERVING thread does -// it: it polls the registry anyway, it never touches the guarded page, and -// without it the first legitimate access would silently disarm the detector -// for the rest of the round. -// * WHAT THE ASSERTION IS. The invariant is "no access AFTER the publish", and -// that is what gets asserted: a round starts with nothing published, so a -// recorded access stamped `published >= 1` is one this waiter made below -// publishFinishedWaiter(). It is deliberately NOT "zero accesses", because -// zero is stricter than the rule and FALSE on a correct waiter: on the -// deferred/"multi" path the waiter calls awaitCompletion(), which locks -// m_completionMu and reads m_completions and m_objectName — all on the -// guarded page, all before it publishes, all legitimate. The fourth round -// below drives exactly that path and REQUIRES at least one such access, so -// the narrowing is not a loophole: it is the difference between the two -// halves of the object's life, and both are checked. (This file asserted -// zero when it landed. It was green only because none of its rounds -// deferred; adding one turns the strict form red on correct code, which is -// how the over-strictness was found.) -// * WHERE THE DEFERRED ROUND IS WEAKER, stated because "we relaxed the -// assertion" is exactly the change that can hide a dead detector. On that -// one round the post-publish half is best-effort rather than deterministic: -// a legitimate access opens the page, the re-arm is a syscall behind, and a -// defect firing a microsecond later slips through. Measured with a read of -// m_objectName added below the publish, and every round forced to run: the -// other four catch it 5 times in 5, the deferred round 0 times in 5 — and a -// variant that spins on the re-arm instead of polling records 4-24 accesses -// per round and still catches it 0 times in 5, so this is not a tuning -// problem. -// IT COSTS NOTHING, because FinishOnExit is ONE piece of code shared by all -// five exit paths. A defect below the publish is the same defect on every -// round, and the rounds that can see it deterministically do (25/25 for the -// whole test, unchanged by the narrowing — see the table). The deferred -// round is here to keep the assertion honest about correct code, not to add -// a fifth copy of the same detection. -// -// HALF TWO — the registry (PublishedWaiterDoesNotTouchTheRegistryAgain). The -// page trick cannot cover m_waiterMu, m_waiters or m_finishedWaiters, because -// publishing has to reach them. That half is caught with bait instead; the -// method is described on the test itself. -// -// Everything is driven through a fake RpcConnectionBase, so there is no socket, -// no host, no event loop timing, and no race: the test decides exactly when the -// call's future is satisfied, which is what lets it arm the page while the -// waiter is parked and disarm it only once the waiter is gone. The two rounds -// that end on a deadline (timed out, deferred) are the exception, and they wait -// out a real clock — the budget is set so the thing that has to happen first -// takes microseconds against hundreds of milliseconds of slack, and the deferred -// round asserts that it did rather than assuming it. -// -// WHAT THE TWO HALVES DO AND DO NOT COVER, measured by rebuilding the file under -// test with each defect and running each suite 5-30 times: -// -// defect below publishFinishedWaiter() here teardown+reaping suites -// ------------------------------------ ---- ----------------------- -// read m_objectName 25/25 0/10 -// read m_conn 10/10 0/10 -// read m_completions 10/10 0/10 -// read m_completionSubscribed 10/10 0/10 -// lock m_mu 25/25 0/10 -// write m_completions under m_completionMu 25/25 - -// call reapFinishedWaiters() again 20/20 9/30 -// read m_stopping 0/10 0/10 -// (publish moved ABOVE the reap) 0/5 12/15 -// no defect 0/30 0/10 -// -// TAKE THE RIGHT-HAND COLUMN AS RATES, NOT AS FRACTIONS. Those suites catch -// these by racing, so a sample is a coin count and not a constant: the re-reap -// cell was first reported here as "2/2", which was a two-run sample printed -// beside 10-40 run samples. Over 30 runs it is 9/30, and another 30-run sample -// gave 12/30 — i.e. roughly one run in three, matching what reapFinishedWaiters' -// own comment says ("about one run in four"). The left-hand column is NOT a -// rate: those are deterministic, and anything below 1 there would be a bug in -// this file rather than bad luck. -// -// The one real gap is m_stopping, the single member that shares the registry's -// page and so cannot be guarded without guarding the publish itself. The one -// defect this suite deliberately leaves alone is the inverted order, which the -// reaping suite's hammer already catches — probabilistically, at 12 runs in 15, -// which is the other half of why observing beats racing. -// -// m_waiterMu / m_waiters / m_finishedWaiters / m_stopping stay private: they are -// reached through the same explicit-instantiation access hole the reaping suite -// uses ([temp.spec] does not check access on the template arguments of an -// explicit instantiation), so the code under test is observed exactly as it -// ships — no friend, no test-only accessor, no #define private public. - -#include - -#include "logos_async_dispatch.h" -#include "logos_call_error.h" - -#include "plain_logos_object.h" -#include "rpc_connection.h" - -#include -#include -#include -#include -#include - -#include -#include -#include -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -using namespace logos::plain; - -namespace { - -// ── reading the waiter registry without touching the production header ────── -template -struct Rob { - friend typename Tag::type get(Tag) { return Member; } -}; - -struct PubWaitersTag { - using type = std::map PlainLogosObject::*; - friend type get(PubWaitersTag); -}; -template struct Rob; - -struct PubWaiterMuTag { - using type = std::mutex PlainLogosObject::*; - friend type get(PubWaiterMuTag); -}; -template struct Rob; - -struct PubFinishedTag { - using type = std::vector PlainLogosObject::*; - friend type get(PubFinishedTag); -}; -template struct Rob; - -struct PubStoppingTag { - using type = std::atomic PlainLogosObject::*; - friend type get(PubStoppingTag); -}; -template struct Rob; - -std::size_t memberOffset(const PlainLogosObject* obj, const void* member) -{ - return static_cast(reinterpret_cast(member) - - reinterpret_cast(obj)); -} - -// ── the access recorder ───────────────────────────────────────────────────── -// -// A guarded page plus a fault handler that logs and then lets the access -// through. Deliberately NOT a crash: a test that dies inside a waiter thread -// reports "signal 11" and nothing else, whereas the interesting part is WHICH -// member was touched and whether the touching thread had already published. - -constexpr int kMaxFaults = 64; - -struct Access { - std::uintptr_t addr = 0; - std::uintptr_t thread = 0; - std::size_t offset = 0; - std::size_t published = 0; // m_finishedWaiters.size() at fault time -}; - -std::atomic gAccessCount{0}; -Access gAccesses[kMaxFaults]; -std::atomic gGuardBase{nullptr}; -std::atomic gGuardLen{0}; -std::atomic gObjBase{nullptr}; -std::atomic gArmed{false}; // false once a fault has let one through -const std::vector* gFinished = nullptr; // on the UNguarded page -struct sigaction gOldSegv; -struct sigaction gOldBus; -bool gHandlersInstalled = false; - -void faultHandler(int /*sig*/, siginfo_t* info, void* /*uctx*/) -{ - char* const base = gGuardBase.load(std::memory_order_acquire); - const std::size_t len = gGuardLen.load(std::memory_order_acquire); - char* const addr = static_cast(info->si_addr); - - if (base == nullptr || addr < base || addr >= base + len) { - // Not our arena: put the original handlers back and return, so the - // faulting instruction re-runs and dies with its real diagnosis rather - // than looping forever in here. - sigaction(SIGSEGV, &gOldSegv, nullptr); - sigaction(SIGBUS, &gOldBus, nullptr); - return; - } - - const int i = gAccessCount.fetch_add(1, std::memory_order_acq_rel); - if (i < kMaxFaults) { - char* const objBase = gObjBase.load(std::memory_order_acquire); - gAccesses[i].addr = reinterpret_cast(addr); - gAccesses[i].thread = reinterpret_cast(pthread_self()); - gAccesses[i].offset = objBase ? static_cast(addr - objBase) : 0; - // On the registry page, which is never guarded — and with a single - // waiter in flight the only writer is the very thread stopped here. - gAccesses[i].published = gFinished ? gFinished->size() : 0; - } - // Let it through: re-arming HERE would need a single-step, since the - // faulting instruction re-runs the moment this returns and would fault - // forever. Unprotect FIRST and only then drop the armed flag, so an - // observer racing to re-arm cannot have its mprotect(PROT_NONE) overwritten - // by this one — the worst interleaving then leaves the page guarded and the - // flag set, which is the truth. - mprotect(base, len, PROT_READ | PROT_WRITE); - gArmed.store(false, std::memory_order_release); -} - -void installHandlers() -{ - if (gHandlersInstalled) return; - struct sigaction sa{}; - sa.sa_sigaction = &faultHandler; - sa.sa_flags = SA_SIGINFO; - sigemptyset(&sa.sa_mask); - ASSERT_EQ(sigaction(SIGSEGV, &sa, &gOldSegv), 0); - ASSERT_EQ(sigaction(SIGBUS, &sa, &gOldBus), 0); - gHandlersInstalled = true; -} - -void resetAccesses() { gAccessCount.store(0, std::memory_order_release); } - -int accessCount() { return gAccessCount.load(std::memory_order_acquire); } - -bool guard(bool on) -{ - char* const base = gGuardBase.load(std::memory_order_acquire); - const std::size_t len = gGuardLen.load(std::memory_order_acquire); - if (!base) return false; - if (mprotect(base, len, on ? PROT_NONE : (PROT_READ | PROT_WRITE)) != 0) - return false; - gArmed.store(on, std::memory_order_release); - return true; -} - -// Put the page back after a recorded access let one through. Called ONLY from -// the observing thread, which never touches the guarded page — from anywhere -// else this would be a way to fault on your own re-arm. -// -// Without it the detector is a ONE-SHOT per round: the first access disarms the -// page and everything after it goes unseen. That was harmless while no correct -// waiter ever touched the page, and stops being harmless the moment one -// legitimately does — the deferred round below. What it buys, honestly: the -// page is blind only from a fault until the next poll, instead of for the rest -// of the round. It is NOT enough to catch a defect that fires a microsecond -// after a legitimate access (measured — see the header), and the reason that is -// acceptable is that FinishOnExit is shared by all five rounds, four of which -// see it deterministically. Keep it anyway: any future legitimate access EARLY -// in a round would otherwise silently switch that round's detector off. -void rearmGuard() -{ - if (gArmed.load(std::memory_order_acquire)) return; - if (accessCount() >= kMaxFaults) return; // nothing left to record - guard(true); -} - -// THE ACTUAL INVARIANT: accesses made below publishFinishedWaiter(). Each round -// starts with m_finishedWaiters empty (the spawn path reaps the previous round's -// id, and the round asserts it), and one waiter runs at a time, so an access -// stamped `published >= 1` is one this waiter made after publishing its own id. -// Everything stamped 0 happened before the publish, where the object is still -// alive by construction and touching it is legal. -int postPublishAccesses(int* firstIndex = nullptr) -{ - const int n = std::min(accessCount(), kMaxFaults); - int count = 0; - for (int i = 0; i < n; ++i) { - if (gAccesses[i].published == 0) continue; - if (count == 0 && firstIndex) *firstIndex = i; - ++count; - } - return count; -} - -// ── a connection that answers exactly when the test says so ───────────────── -class ScriptedConn : public RpcConnectionBase { -public: - void start() override {} - void stop(const std::string& = "stopped") override { m_open = false; } - bool isOpen() const override { return m_open; } - - std::future sendCall(CallMessage msg) override - { - auto p = std::make_shared>(); - auto f = p->get_future(); - std::lock_guard g(m_mu); - m_pending[msg.id] = std::move(p); - m_lastId = msg.id; - return f; - } - - std::future sendMethods(MethodsMessage msg) override - { - std::promise p; - MethodsResultMessage r; - r.id = msg.id; - r.ok = true; - p.set_value(std::move(r)); - return p.get_future(); - } - - void sendSubscribe(SubscribeMessage, std::function) override {} - void sendUnsubscribe(UnsubscribeMessage) override {} - void sendEvent(EventMessage) override {} - void sendToken(TokenMessage) override {} - void setErrorHandler(ErrorHandler) override {} - std::uint64_t nextId() override { return m_nextId++; } - - std::uint64_t lastId() - { - std::lock_guard g(m_mu); - return m_lastId; - } - - // Satisfy the call's future. `ok == false` produces the wire-error shape. - void answer(std::uint64_t id, bool ok) - { - ResultMessage r; - r.id = id; - r.ok = ok; - if (ok) { - r.value = RpcValue(std::string("pong")); - } else { - r.err = "provider said no"; - r.errCode = "CALL_FAILED"; - } - deliver(id, std::move(r)); - } - - // Satisfy it with a "multi" provider's PENDING SENTINEL — ok, but the value - // says "the real result comes later, as a completion event keyed by this - // id". Nothing ever pushes that completion here (this connection has no - // event plumbing at all), so the waiter parks in awaitCompletion() until the - // call's own deadline, which is what the deferred round wants: a CORRECT - // waiter legitimately holding m_completionMu and reading m_completions and - // m_objectName — all on the guarded page — before it publishes. - void answerDeferred(std::uint64_t id, const std::string& callId) - { - RpcMap m; - m.emplace(logos::pendingCallKey().toStdString(), RpcValue(callId)); - ResultMessage r; - r.id = id; - r.ok = true; - r.value = RpcValue(std::move(m)); - deliver(id, std::move(r)); - } - -private: - void deliver(std::uint64_t id, ResultMessage r) - { - std::shared_ptr> p; - { - std::lock_guard g(m_mu); - const auto it = m_pending.find(id); - if (it == m_pending.end()) return; - p = std::move(it->second); - m_pending.erase(it); - } - p->set_value(std::move(r)); - } - - std::mutex m_mu; - std::map>> m_pending; - std::uint64_t m_lastId = 0; - std::atomic m_nextId{1}; - std::atomic m_open{true}; -}; - -// ── the arena ─────────────────────────────────────────────────────────────── -// -// Two pages. The object straddles the boundary between them, placed so that -// m_waiterMu begins exactly on the second page: page 1 is then everything the -// waiter must never touch, and page 2 is the registry it legitimately does. -class GuardedObject { -public: - GuardedObject() - { - m_pageSize = static_cast(sysconf(_SC_PAGESIZE)); - m_conn = std::make_shared(); - - // The offset of m_waiterMu, measured on a throwaway instance so the - // real one can be placed against it. - std::size_t split = 0; - { - PlainLogosObject probe("probe", m_conn); - split = memberOffset(&probe, &(probe.*get(PubWaiterMuTag{}))); - } - m_split = split; - if (m_split % alignof(PlainLogosObject) != 0) - m_split -= m_split % alignof(PlainLogosObject); - - void* mem = mmap(nullptr, m_pageSize * 2, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - if (mem == MAP_FAILED) return; - m_arena = static_cast(mem); - - char* const at = m_arena + m_pageSize - m_split; - m_obj = new (at) PlainLogosObject("guarded_module", m_conn); - - m_waiterMuOffset = memberOffset(m_obj, &(m_obj->*get(PubWaiterMuTag{}))); - gFinished = &(m_obj->*get(PubFinishedTag{})); - gObjBase.store(reinterpret_cast(m_obj), std::memory_order_release); - gGuardBase.store(m_arena, std::memory_order_release); - gGuardLen.store(m_pageSize, std::memory_order_release); - } - - ~GuardedObject() - { - guard(false); - gGuardBase.store(nullptr, std::memory_order_release); - gObjBase.store(nullptr, std::memory_order_release); - gFinished = nullptr; - // NOT release(): that ends in `delete this`, and this object was never - // new'd. The destructor is the same teardown minus the free. - if (m_obj) m_obj->~PlainLogosObject(); - if (m_arena) munmap(m_arena, m_pageSize * 2); - } - - // The guarded region is [obj, obj + split). What has to hold is that the - // split lands ON the page boundary and NOT past m_waiterMu — the registry - // and its mutex must stay writable, or publishing itself would fault. The - // split is the member offset rounded down to the object's alignment, so on - // a platform where m_waiterMu is not aligned to it the guard simply stops a - // few bytes of padding short, which is still correct. - bool ok() const - { - return m_obj != nullptr && m_arena != nullptr - && m_split > 0 && m_split <= m_waiterMuOffset - && reinterpret_cast(m_obj) + m_split == m_arena + m_pageSize - && sizeof(PlainLogosObject) < m_pageSize; - } - - PlainLogosObject* obj() const { return m_obj; } - ScriptedConn* conn() const { return m_conn.get(); } - std::size_t split() const { return m_split; } - std::size_t waiterMuOffset() const { return m_waiterMuOffset; } - - std::size_t waiterCount() const - { - std::lock_guard g(m_obj->*get(PubWaiterMuTag{})); - return (m_obj->*get(PubWaitersTag{})).size(); - } - - std::size_t publishedCount() const - { - std::lock_guard g(m_obj->*get(PubWaiterMuTag{})); - return (m_obj->*get(PubFinishedTag{})).size(); - } - - void forceStopping() - { - (m_obj->*get(PubStoppingTag{})).store(true, std::memory_order_release); - } - -private: - std::size_t m_pageSize = 0; - std::size_t m_split = 0; - std::size_t m_waiterMuOffset = 0; - char* m_arena = nullptr; - PlainLogosObject* m_obj = nullptr; - std::shared_ptr m_conn; -}; - -QCoreApplication* ensureAppForGuard() -{ - static int argc = 0; - static char* argv[] = { nullptr }; - if (!QCoreApplication::instance()) - new QCoreApplication(argc, argv); - return QCoreApplication::instance(); -} - -void pumpMs(int ms) -{ - QElapsedTimer t; - t.start(); - while (t.elapsed() < ms) - QCoreApplication::processEvents(QEventLoop::AllEvents, 2); -} - -// Wait — WITHOUT pumping, so nothing of ours runs on the object — until the -// waiter has published and had time to unwind past it. -// -// Doubles as the re-armer. Both things it does touch only the registry page -// (publishedCount takes m_waiterMu) or the mapping itself, never the guarded -// page, so this is the one place from which putting the guard back is safe. -bool waitForPublish(const GuardedObject& g, int budgetMs) -{ - QElapsedTimer t; - t.start(); - while (t.elapsed() < budgetMs) { - rearmGuard(); - if (g.publishedCount() > 0) return true; - QThread::usleep(200); - } - return false; -} - -// The publish is the waiter's last act, so anything it does below it lands in -// the moments right after — which is where the guard has to be up. Poll instead -// of sleeping so a page disarmed by a legitimate pre-publish access is back -// before then. -void settleGuarded(int ms) -{ - QElapsedTimer t; - t.start(); - while (t.elapsed() < ms) { - rearmGuard(); - QThread::usleep(200); - } -} - -std::string describe(const Access& a, std::size_t split, std::uintptr_t testThread) -{ - char buf[320]; - std::snprintf(buf, sizeof(buf), - "offset %zu (the guarded state page runs to %zu, where the " - "waiter registry begins), from %s thread, %zu waiter id(s) " - "already published", - a.offset, split, - a.thread == testThread ? "the TEST" : "a non-test", - a.published); - return std::string(buf); -} - -std::uintptr_t selfThread() -{ - return reinterpret_cast(pthread_self()); -} - -// A thread parked until the test lets it go — the bait a reaper picks up and -// then blocks on, which is what holds the window open in the second test. -class Gate { -public: - void wait() - { - std::unique_lock lk(m_mu); - m_cv.wait(lk, [this] { return m_open; }); - } - void open() - { - { - std::lock_guard g(m_mu); - m_open = true; - } - m_cv.notify_all(); - } - -private: - std::mutex m_mu; - std::condition_variable m_cv; - bool m_open = false; -}; - -const char* kGuardToken = "guard-token"; - -} // namespace - -class PlainWaiterPublishIsLastTest : public ::testing::Test { -protected: - void SetUp() override - { - ensureAppForGuard(); - installHandlers(); - resetAccesses(); - } -}; - -// ── 0. the detector has to be able to fire ────────────────────────────────── -// -// The whole suite is an assertion that a counter stays at zero, which is the -// shape of test that passes just as happily when the mechanism under it is -// dead. So prove the mechanism first: a deliberate read of the object's state -// page, from a thread that is not the test's, must be recorded — with the -// address, the thread, and the published-id count that the real check reads. -TEST_F(PlainWaiterPublishIsLastTest, DetectorRecordsADeliberateTouch) -{ - GuardedObject g; - ASSERT_TRUE(g.ok()) << "arena layout: split=" << g.split() - << " m_waiterMu at " << g.waiterMuOffset() - << " sizeof=" << sizeof(PlainLogosObject); - - resetAccesses(); - ASSERT_TRUE(guard(true)) << "mprotect failed: " << strerror(errno); - - std::uintptr_t toucherThread = 0; - std::thread toucher([&] { - toucherThread = reinterpret_cast(pthread_self()); - // Any byte of the state page will do; this is m_objectName's first. - volatile const char* p = reinterpret_cast(g.obj()); - (void)*p; - }); - toucher.join(); - - guard(false); - - ASSERT_GE(accessCount(), 1) - << "the guard page recorded nothing for a read that certainly happened " - "— the detector is dead and every other test in this file is vacuous"; - EXPECT_EQ(gAccesses[0].thread, toucherThread); - EXPECT_NE(gAccesses[0].thread, selfThread()); - EXPECT_EQ(gAccesses[0].offset, 0u); - std::cout << " detector live: " - << describe(gAccesses[0], g.split(), selfThread()) << std::endl; -} - -// ── 1. the invariant ──────────────────────────────────────────────────────── -// -// Five waiters: one per return out of the lambda (answered, rejected, timed out, -// cancelled) plus the deferred detour that awaitCompletion() adds to the -// answered one. Every one of them ends in the same FinishOnExit guard, and it is -// the guard that has to keep its hands off the object once it has published. -// -// Each round arms the state page only after the call has returned (the caller's -// own accesses — m_conn, m_objectName, ensureCompletionSub — are legitimate and -// happen there), and disarms it only once the waiter has published and unwound. -// For that whole window the waiter is the only thing running against the object. -// -// WHAT IS ASSERTED IS "NOTHING AFTER THE PUBLISH", not "nothing at all". Four of -// the five rounds do touch the page zero times, because objectName and method -// are copied into the closure precisely so the waiter needs nothing from the -// object. The DEFERRED round does not, and correctly so: a "multi" provider's -// pending sentinel sends the waiter into awaitCompletion(), which locks -// m_completionMu and reads m_completions and m_objectName — guarded page, before -// the publish, entirely legal. Asserting zero would make this file red on -// correct code, so it asserts what the rule actually says: no access stamped -// `published >= 1`. The deferred round then requires at least one stamped 0, so -// the looser predicate cannot be satisfied by the path simply not running. -// -// Pre-fix — i.e. with any member access added below publishFinishedWaiter — this -// records an access with published >= 1 and fails naming the offset. -TEST_F(PlainWaiterPublishIsLastTest, WaiterTouchesNothingOnTheStatePageAfterPublishing) -{ - GuardedObject g; - ASSERT_TRUE(g.ok()) << "arena layout: split=" << g.split() - << " m_waiterMu at " << g.waiterMuOffset() - << " sizeof=" << sizeof(PlainLogosObject); - - struct Round { - const char* what; - int timeoutMs; - enum { Answer, Fail, Timeout, Deferred, Cancel } how; - // Rounds where a CORRECT waiter must reach the guarded page before it - // publishes. Only the deferred path does; requiring it is what keeps - // that round from passing vacuously if it stops deferring. - bool touchesBeforePublishing; - }; - const Round rounds[] = { - { "a call that ANSWERS", 5000, Round::Answer, false }, - { "a call the provider REJECTS", 5000, Round::Fail, false }, - { "a call that TIMES OUT", 150, Round::Timeout, false }, - // The pending-sentinel path. The budget is the awaitCompletion deadline - // (no completion is ever pushed, so it runs out) and also the slack the - // future has to be satisfied in — microseconds are needed, 400ms given. - { "a DEFERRED call (pending sentinel)", 400, Round::Deferred, true }, - { "a call CANCELLED by teardown", 5000, Round::Cancel, false }, - }; - constexpr int kRounds = static_cast(sizeof(rounds) / sizeof(rounds[0])); - - int delivered = 0; - for (const Round& r : rounds) { - resetAccesses(); - - // Owned by the callback, not by this frame: a bail-out below leaves the - // waiter to be cancelled in teardown, and its callback is delivered on a - // LATER event-loop iteration — after this frame is gone. A captured - // `&got` would be a genuine use-after-free on the way out of a failing - // test, which is a poor advertisement for a file about use-after-free. - auto got = std::make_shared>(0); - g.obj()->callMethodAsyncWithError( - QString::fromLatin1(kGuardToken), QStringLiteral("ping"), - QVariantList{ QVariant(1) }, r.timeoutMs, - [got](QVariant, const logos::CallError&) { got->fetch_add(1); }); - - // The waiter is registered (that happens under m_waiterMu inside the - // call) and parked in waitForResult: nothing can satisfy its future - // until the line below. Safe to close the state page over it. - ASSERT_GT(g.waiterCount(), 0u) << r.what << ": no waiter registered"; - // The spawn path above reaps the previous round's id, so nothing is - // published as this round starts — which is what makes `published >= 1` - // mean "after THIS waiter's publish" rather than "after some earlier - // waiter's". - ASSERT_EQ(g.publishedCount(), 0u) - << r.what << ": a previous round's waiter id is still published, so " - "the after-the-publish stamp no longer means what it says"; - ASSERT_TRUE(guard(true)) << "mprotect failed: " << strerror(errno); - - switch (r.how) { - case Round::Answer: g.conn()->answer(g.conn()->lastId(), true); break; - case Round::Fail: g.conn()->answer(g.conn()->lastId(), false); break; - case Round::Timeout: break; // let the deadline elapse - case Round::Deferred: - g.conn()->answerDeferred(g.conn()->lastId(), "deferred-call-1"); - break; - case Round::Cancel: g.forceStopping(); // registry page, not guarded - break; - } - - const bool published = waitForPublish(g, 10000); - // The publish is the waiter's last act; give the thread room to unwind - // past it, which is where a stray access would land — with the guard - // kept up for all of it. - settleGuarded(30); - - const int recorded = accessCount(); - int firstAfter = 0; - const int afterPublish = postPublishAccesses(&firstAfter); - guard(false); - - EXPECT_TRUE(published) << r.what << ": the waiter never published"; - ASSERT_EQ(afterPublish, 0) - << r.what << ": the object's state page was touched AFTER this " - "waiter published its id, while only the waiter was running — " - << describe(gAccesses[firstAfter], g.split(), selfThread()) << ".\n" - << "That access is below publishFinishedWaiter() in the FinishOnExit " - "guard, and it breaks the guarantee stopAndJoinWaiters() " - "documents: a reaper joining this waiter outside m_waiterMu has " - "already taken it out of m_waiters, so teardown does not wait for " - "it and release() goes on to `delete this` while it runs. No " - "SUPPORTED caller can reach that reaper today (see the header), " - "so this is the regression test for an invariant the design " - "rests on — not the report of a live use-after-free. Publishing " - "must stay the last thing a waiter does."; - - // A round that must have gone through the guarded page has to prove it, - // or it is not testing the path it names. - if (r.touchesBeforePublishing) { - ASSERT_GT(recorded, 0) - << r.what << ": nothing on the guarded page was touched, so " - "awaitCompletion() never ran — the call did not actually " - "defer (a slow machine can turn this into a plain timeout) " - "and this round proves nothing about the deferred path"; - } else { - EXPECT_EQ(recorded, 0) - << r.what << ": the guarded page was touched at all, which on " - "this path a correct waiter never does — " - << describe(gAccesses[0], g.split(), selfThread()); - } - - // Drain the queued callback so the round is provably complete. - pumpMs(60); - delivered += got->load(); - std::cout << " " << r.what << ": published, " << recorded - << " access(es) to the state page, " << afterPublish - << " of them after the publish" << std::endl; - - if (r.how == Round::Cancel) break; // m_stopping never clears - } - - EXPECT_EQ(delivered, kRounds) - << "a callback went missing; the rounds did not all run"; -} - -// ── 2. the other half of the object: the registry itself ──────────────────── -// -// The guard page above cannot cover m_waiterMu, m_waiters or m_finishedWaiters — -// publishing has to be able to reach them. So the one access it cannot see is a -// waiter REAPING AGAIN after publishing, which is precisely the mistake -// publishFinishedWaiter's comment calls out: "Nothing the waiter does may follow -// it, its own reap least of all." -// -// Nothing else catches that either. It is invisible to the reaping suite (the -// self-join guard absorbs it into a leaked registry entry rather than a crash) -// and to the teardown suite, and racing a reaper against it catches it roughly -// one run in three — 9 in 30 measured here, 12 in 30 on another 30-run sample, -// against the "2/2" this file first printed from a two-run sample. That spread -// IS the argument for not racing: at that rate a green run means nothing, and -// the bait below is deterministic. -// -// Instead it uses the reaper's own shape against it. reapFinishedWaiters() -// joins OUTSIDE m_waiterMu, so a waiter that has picked up somebody else's -// finished thread sits in that join with the lock free — a window the test can -// hold open for as long as it likes, because the thread being joined is one the -// test planted and can keep parked: -// -// 1. plant BAIT 1 — a parked thread registered under a synthetic id, and that -// id published — while the call is still in flight; -// 2. answer the call. The waiter's exit guard reaps, takes bait 1, and parks -// in join(bait 1), holding no lock; -// 3. plant BAIT 2 the same way. There is no hurry: the waiter is parked; -// 4. release bait 1. The waiter finishes its reap and publishes; -// 5. bait 2 is now the tell. A waiter that is done never looks at the registry -// again, so bait 2 must still be registered. A waiter that reaps once more -// after publishing takes it. -// -// Bait 1 doubles as a check that the reap really does join with the lock free: -// step 3 needs m_waiterMu while the join is in progress, and says so if it -// cannot get it. -// -// A NOTE ON BAILING OUT, because the bait is a trap for the test as much as for -// the waiter. Every planted thread is parked until this test opens its gate, and -// ~PlainLogosObject joins whatever is still registered — so an early return that -// skips a gate.open() does not fail, it WEDGES. Measured on the first version of -// this test: remove the reap from the exit guard, ASSERT_TRUE(tookBait1) fires -// and prints — and then the destructor blocks forever joining a thread nobody -// will release, so the run ends as a timeout kill (exit 124) with no test result -// at all. With the gates on a scope guard the same break reports the same -// assertion and exits 1 in 10s, which is the tryWithRegistry budget and not a -// hang. -// -// Not hypothetical, either: the TODO above the waiter (fold the wait into the -// shared Asio io_context) moves where reaping happens, which is exactly the edit -// that makes tookBait1 false. So the gates are opened by a scope guard on EVERY -// exit path, and they are declared BEFORE the object so they outlive the -// teardown that joins the threads waiting on them. -TEST_F(PlainWaiterPublishIsLastTest, PublishedWaiterDoesNotTouchTheRegistryAgain) -{ - // Declared first, destroyed last: ~GuardedObject joins the planted threads, - // and a thread parked on an already-destroyed condition_variable would be - // its own use-after-free. - Gate gate1; - Gate gate2; - - GuardedObject g; - - // Destroyed BEFORE g (declared after it), so by the time the object's - // teardown joins the bait, both gates are open. A failed precondition below - // now FAILS — fast, named, with a normal exit code — instead of wedging. - struct OpenGatesOnExit { - Gate& first; - Gate& second; - ~OpenGatesOnExit() { first.open(); second.open(); } - } openGatesOnExit{gate1, gate2}; - - ASSERT_TRUE(g.ok()); - - std::mutex& mu = g.obj()->*get(PubWaiterMuTag{}); - auto& registry = g.obj()->*get(PubWaitersTag{}); - auto& finished = g.obj()->*get(PubFinishedTag{}); - - // Far above anything m_nextWaiterId will reach in this test. - constexpr std::uint64_t kBait1 = 1ull << 40; - constexpr std::uint64_t kBait2 = (1ull << 40) + 1; - - // Every registry read below is a TRY-lock: if the reap ever started joining - // with m_waiterMu held, a blocking lock here would hang the suite instead of - // reporting it. - // - // THE LOCK IS DROPPED BEFORE THE SLEEP, and that inner scope is the whole - // reason this probe terminates. Every condition it waits for is one only the - // WAITER can produce, and the waiter produces all of them under this very - // mutex — reapFinishedWaiters() takes it to erase the bait, - // publishFinishedWaiter() takes it to append the id. Sleeping inside the - // lock's scope therefore does not poll the waiter, it BLOCKS it: the probe - // would hold m_waiterMu for the whole 200us and yield it only for the few - // nanoseconds between the unlock and the next try_to_lock, and std::mutex - // hands off by barging rather than FIFO, so the waiter only gets in when it - // happens to be running on another core at that instant. That is a lottery - // with no bound on it, and it is what made this test flaky: - // - // * measured with the lock held across the sleep, by counting iterations: - // the probe acquired the mutex on essentially EVERY iteration (455/455, - // 567/567, ~0 try-lock misses) while the waiter needed anywhere from 1 - // to 700+ attempts to get a single acquisition through; - // * the wall clock is that count times the cost of an iteration, and on a - // loaded or virtualised runner the 200us sleep really costs ~20ms, so a - // few hundred attempts is the 10s budget. Under deliberate CPU - // oversubscription both CI failures reproduced from this one cause, 3 - // runs in 25: "the waiter's exit guard never reaped the planted entry" - // (the reap's acquisition lost) and "the waiter never published" (the - // publish's did); - // * on an idle many-core box the woken waiter is dispatched fast enough to - // win within a few dozen attempts, which is why this only ever failed in - // CI and never locally. - // - // With the lock released the waiter simply blocks on it and takes it the - // moment the probe lets go, so each phase completes in one or two - // iterations instead of hundreds. Nothing about what is being tested moves: - // the window this test needs is held open by gate1, not by timing, and the - // budget below stays a backstop rather than the mechanism. - auto tryWithRegistry = [&](const std::function& fn, int budgetMs, - bool* lockedAtLeastOnce) -> bool { - QElapsedTimer t; - t.start(); - while (t.elapsed() < budgetMs) { - { - std::unique_lock lk(mu, std::try_to_lock); - if (lk.owns_lock()) { - if (lockedAtLeastOnce) *lockedAtLeastOnce = true; - if (fn()) return true; - } - } - QThread::usleep(200); - } - return false; - }; - - auto plant = [&](std::uint64_t id, Gate& gate) { - std::lock_guard lk(mu); - registry.emplace(id, std::thread([&gate] { gate.wait(); })); - finished.push_back(id); - }; - - // 1. a call that cannot finish until the test says so. The counter is owned - // by the callback for the same reason as in the test above: on a bail-out - // this callback is delivered after the frame is gone. - auto got = std::make_shared>(0); - g.obj()->callMethodAsyncWithError( - QString::fromLatin1(kGuardToken), QStringLiteral("ping"), - QVariantList{ QVariant(1) }, 20000, - [got](QVariant, const logos::CallError&) { got->fetch_add(1); }); - - plant(kBait1, gate1); - - // 2. let it finish. Its exit guard reaps first, so it takes bait 1. - g.conn()->answer(g.conn()->lastId(), true); - - bool locked = false; - const bool tookBait1 = tryWithRegistry( - [&] { return registry.count(kBait1) == 0; }, 10000, &locked); - ASSERT_TRUE(locked) - << "m_waiterMu was never free while the waiter's reap ran — a reap that " - "joins while holding it is the deadlock reapFinishedWaiters() " - "documents"; - ASSERT_TRUE(tookBait1) - << "the waiter's exit guard never reaped the planted entry, so this " - "probe never got its window; the test needs updating, not the code"; - - // 3. the waiter is now inside join(bait 1), holding nothing. - plant(kBait2, gate2); - - // 4. release it. Reap finishes, then it publishes — and that must be that. - gate1.open(); - - const bool published = tryWithRegistry( - [&] { - for (const std::uint64_t id : finished) - if (id != kBait2) return true; - return false; - }, - 10000, nullptr); - EXPECT_TRUE(published) << "the waiter never published"; - - // 5. anything it did after publishing has had ample room to happen. - QThread::msleep(200); - - bool bait2Gone = false; - { - std::lock_guard lk(mu); - bait2Gone = registry.count(kBait2) == 0; - } - - EXPECT_FALSE(bait2Gone) - << "a waiter that had already published came back and reaped again: the " - "entry planted while it was parked mid-join is gone from m_waiters, " - "so something below publishFinishedWaiter() in the FinishOnExit guard " - "still touches the object.\n" - "That breaks the guarantee stopAndJoinWaiters() documents. A reaper " - "that has taken this waiter out of m_waiters and is joining it " - "outside the lock is not in the map stopAndJoinWaiters() swapped, so " - "teardown does not wait for it and release() goes on to `delete this` " - "while the waiter is still running. No supported caller can reach " - "that reaper today — see the header — so this is a regression test " - "for an invariant the design rests on, not the report of a live " - "use-after-free. Publishing has to stay the last thing a waiter does."; - - // Cleanup: free bait 2 whoever ended up holding it, and retire it if the - // registry still has it. - gate2.open(); - { - std::lock_guard lk(mu); - const auto it = registry.find(kBait2); - if (it != registry.end()) { - std::thread t = std::move(it->second); - registry.erase(it); - if (t.joinable()) t.join(); - } - finished.clear(); - } - - pumpMs(50); - EXPECT_EQ(got->load(), 1) << "the call did not deliver exactly once"; - std::cout << " bait planted mid-join survived the publish: the waiter never " - "came back to the registry" << std::endl; -} diff --git a/tests/protocol/test_plain_waiter_reaping.cpp b/tests/protocol/test_plain_waiter_reaping.cpp index 094d321..8e0d1cd 100644 --- a/tests/protocol/test_plain_waiter_reaping.cpp +++ b/tests/protocol/test_plain_waiter_reaping.cpp @@ -1,3 +1,16 @@ +// RETARGETED BY THE io_context FOLD — read this before the history below. +// +// The mechanism this file was written against is gone: there are no waiter +// threads, no publish list and no reaping, because an async call is no longer a +// thread. What it MEASURES is unchanged, which is why the file survived rather +// than being deleted with the code it was written for — retention must not grow +// with call count, a burst that goes idle must drain with no further call, and +// teardown must still deliver exactly once. It now reads CallState::inflight +// (see the accessor further down) instead of m_waiters, and the numbers it +// prints are in-flight calls rather than threads. The file name, and everything +// below this banner, is the history of the defect it was built for. +// +// ───────────────────────────────────────────────────────────────────────────── // A long-lived PlainLogosObject must not accumulate the waiters of calls that // have already finished. // @@ -101,32 +114,39 @@ using namespace logos::plain; namespace { -// ── reading m_waiters without touching the production header ──────────────── +// ── reading the in-flight registry without touching the production header ─── +// +// RETARGETED BY THE io_context FOLD. This file was written against +// m_waiters/m_finishedWaiters — a map of std::thread plus a publish list, which +// existed only because a thread cannot join itself. There are no threads any +// more: an async call is a shared_ptr in CallState::inflight, put +// there when the call is issued and erased by the one delivery it gets. +// +// The CLAIMS are unchanged and are the reason this file survives the rework +// rather than being deleted with the mechanism it was written for: retention +// must not scale with call count, a burst that goes idle must drain with no +// further call, and teardown must still deliver exactly once. What changes is +// that they now hold for a much duller reason — an entry's lifetime IS the +// call's — so the deadlock test below is pinning something that can no longer +// happen (nothing joins anything) and is kept as a cheap tripwire. template struct Rob { friend typename Tag::type get(Tag) { return Member; } }; -struct WaitersTag { - using type = std::map PlainLogosObject::*; - friend type get(WaitersTag); +struct StateTag { + using type = std::shared_ptr PlainLogosObject::*; + friend type get(StateTag); }; -template struct Rob; +template struct Rob; -struct WaiterMuTag { - using type = std::mutex PlainLogosObject::*; - friend type get(WaiterMuTag); -}; -template struct Rob; - -// Taken under the object's OWN mutex — the one the registration path holds — -// so this is a consistent read, not a torn one. -size_t waiterCount(PlainLogosObject* obj) +// Taken under the state's OWN mutex — the one the registration path holds — so +// this is a consistent read, not a torn one. +size_t inflightCount(PlainLogosObject* obj) { - auto& mu = obj->*get(WaiterMuTag{}); - auto& m = obj->*get(WaitersTag{}); - std::lock_guard g(mu); - return m.size(); + auto& st = obj->*get(StateTag{}); + std::lock_guard g(st->mu); + return st->inflight.size(); } // Answers `ping` immediately — every call in the retention tests COMPLETES, @@ -475,11 +495,11 @@ TEST_F(PlainWaiterReapingTest, SequentialCompletedCallsDoNotAccumulateWaiters) }); pumpUntilTotal(d, i + 1, 10000); ASSERT_EQ(d.total.load(), i + 1) << "call " << i << " never delivered"; - peak = std::max(peak, waiterCount(plain)); + peak = std::max(peak, inflightCount(plain)); } - const size_t finalCount = waiterCount(plain); - std::cout << " " << kCalls << " sequential completed calls -> m_waiters peak=" + const size_t finalCount = inflightCount(plain); + std::cout << " " << kCalls << " sequential completed calls -> in-flight peak=" << peak << " final=" << finalCount << std::endl; EXPECT_EQ(d.errors.load(), 0) << "a completed call reported an error"; @@ -531,15 +551,15 @@ TEST_F(PlainWaiterReapingTest, ConcurrentCompletedCallsStayBoundedByInFlight) d.record(i, e); }); } - peak = std::max(peak, waiterCount(plain)); + peak = std::max(peak, inflightCount(plain)); pumpUntilTotal(d, issued - kInflight + 1, 10000); } pumpUntilTotal(d, kCalls, 20000); pump(200); // a duplicate delivery would land here - const size_t finalCount = waiterCount(plain); + const size_t finalCount = inflightCount(plain); std::cout << " " << kCalls << " calls at " << kInflight - << " in flight -> m_waiters peak=" << peak + << " in flight -> in-flight registry peak=" << peak << " final=" << finalCount << std::endl; EXPECT_EQ(d.total.load(), kCalls); @@ -571,9 +591,12 @@ TEST_F(PlainWaiterReapingTest, ConcurrentCompletedCallsStayBoundedByInFlight) // number" into kBurst exactly — see WHY THE PROVIDER IS GATED. // // So the bound is read here with NO further call: the burst has to have drained -// itself. What remains is whatever published after the FINAL reap — at minimum -// the last waiter to finish, which by construction has nobody behind it to -// collect it. +// itself. RETARGETED BY THE FOLD, and this is where the two mechanisms differ +// most: there is no last reap and no last exit batch, because an entry's +// lifetime IS its call's. What remains after a burst that all completed is +// therefore ZERO, not "one or two", and the interesting quantity moves to the +// OTHER end of the test — the peak, which the gate below makes a measured 800 +// concurrent async calls carrying no threads at all. // // WHY THE PROVIDER IS GATED, which is the whole design of this test. "Issue 800 // calls in a loop and hope they overlap" is not an experiment, it is a race @@ -606,39 +629,36 @@ TEST_F(PlainWaiterReapingTest, ConcurrentCompletedCallsStayBoundedByInFlight) // counter, not inferred. The burst is then concurrent by construction on every // platform: 800/800 in flight, measured, on both, at every load level tried. // -// AND THE DRAIN IS PACED, which is the other half and is NOT the same thing. The -// first version of this released all 800 at once, and on Linux that replaced one -// scheduling artefact with another: 800 threads become runnable on 6 cores, and -// because a waiter reaps and only THEN publishes, a reaper that collects a large -// batch sits in its join loop while everyone behind it publishes — so the last -// exit batch is enormous. Correct code, 800 released together, 20 runs: +// THE PACED DRAIN BELOW IS INHERITED AND, ON THIS BRANCH, NOT LOAD-BEARING. It +// is carried over from the base because releasing all 800 at once mattered +// THERE: waiters reap and only then publish, so a reaper that collected a large +// batch sat in its join loop while everyone behind it published, and correct +// code left 2-389 on a 6-core Linux box against the defect's 800 — a 2x +// separation, useless as a detector. Here there is nothing to reap and nothing +// to serialize, and the residue is 0 at any pace: measured with kRelease set to +// kBurst — one release, all 800 answered together — it is 0 on all 30 runs per +// platform, the same as at 16. It stays because the structure is worth keeping +// identical to the base's while both branches are live, and because a paced +// drain checks the entries leaving progressively rather than all at the end. // -// macOS 1 every run Linux 2-389 (median ~190) +// MEASURED, this gated burst, CallState::inflight when it goes idle. Numbers, n +// and margins are at the assertion below; the shape is: // -// That is not a defect, it is what the exit guard can and cannot do against a -// thundering herd, and it is recorded in plain_logos_object.h next to the -// mechanism. It is useless as a DETECTOR, because the defect's value (800) is -// only ~2x it. So the gate is opened kRelease at a time, each step awaited: the -// waiters then retire each other the way a real drain does, and the residue is -// the last step's exit batch instead of the burst's. Measured across release -// steps, correct code on Linux, idle: step 1 → 1, step 8 → 1-3, step 16 → 1-2, -// step 32 → 1-3, step 64 → 1-55, step 200 → 1-142, step 800 → 2-389. The knee is -// well above the step chosen below, and a smaller step buys nothing but runtime -// (step 8 costs 3x step 32 on a 64x-oversubscribed box). +// this code a delivery that does not erase its entry +// macOS 0 800 (exactly, every run) +// Linux, any load 0 800 (exactly, every run) // -// MEASURED, this gated burst, m_waiters when it goes idle. Numbers, n and -// margins are at the assertion below; the shape is: +// The defect arm here is NOT the base's — the exit-guard reap it removed does +// not exist any more — but the shape is the same one and it is the closest +// mechanism-appropriate inversion: drop `st->inflight.erase(id)` from +// AsyncCall::deliver(), so a completed call keeps its registration. That gives +// kBurst exactly, on both platforms, and 801 after the follow-up call at the end +// of this test. The separation is not statistical on either side. // -// this code reaping only on the spawn path -// macOS 1-2 800 (exactly, every run) -// Linux, any load 1-7 800 (exactly, every run) -// -// The defect's 800 is not "high", it is the whole burst: with no exit-guard reap -// and no further spawn, nothing on any platform ever removes an entry, so the -// count is kBurst exactly and the separation is not statistical. 800 live waiter -// threads at once is also the peak this now reaches where the ungated version -// reached ~150 on Linux, and that is affordable: the stacks are lazily faulted, -// so peak RSS for the whole run is 29MiB on Linux and 32MiB on macOS. +// AND THE PEAK IS THE CLAIM NOW. 800 async calls outstanding at once is what +// this branch exists to make cheap: on the base, that reading is 800 live +// std::threads; here it is 800 shared_ptr on the shared io_context +// and no thread per pending RPC at all. TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) { LiveHost host; @@ -655,21 +675,20 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) // ~1s in practice. Every wait in this test is already bounded — the gate // self-releases, the pumps have budgets, the calls have timeouts — so this - // is the backstop for a wedge INSIDE the code under test (a reaper deadlocked - // against a publisher), which none of those would catch. + // is the backstop for a wedge INSIDE the code under test, which none of + // those would catch. Cheap, and kept across the fold for that reason. Watchdog watchdog("BurstThatGoesIdleDrainsWithoutAnotherCall", 120000); // Issued in one go, with no pumping in between, against a CLOSED gate: no // reply can be produced until every one of them is outstanding. constexpr int kBurst = 800; // Gated calls released per step of the drain, each step awaited before the - // next. See the header comment: this is what keeps the residue the last - // step's exit batch rather than the burst's. + // next. Inherited from the base and not load-bearing here — see the header + // comment; the residue is 0 at any step, including one. constexpr int kRelease = 16; - // The residue cannot exceed what the last release step can leave behind, so - // the bound is stated against THAT and not against kBurst — 4x the step, - // sized at the assertion below. - constexpr size_t kDrained = 4 * kRelease; + // A small CONSTANT, because on this branch the quantity it bounds is a + // registry that a completed call has already left. Sized at the assertion. + constexpr size_t kDrained = 8; Deliveries d(kBurst); // Declared AFTER `d`, so it runs BEFORE it. The tail of this test used to be // a bare `obj->release(); pump(50);` on the happy path, which a FATAL @@ -679,8 +698,8 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) // run them against a destroyed Deliveries. That is the one way this test // could answer a failure with a crash instead of a verdict, and the // assertions below (a gate that never opened, a burst that did not all - // complete) are exactly the ones that would trigger it. release() cancels - // and JOINS every waiter, and the pump behind it runs what they posted — + // complete) are exactly the ones that would trigger it. release() resolves + // every outstanding call, and the pump behind it runs what they posted — // both while `d` is still alive. struct ReleaseOnExit { LogosObject* obj; @@ -702,13 +721,14 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) // it does not depend on the client, on who reaps what, or on the Qt // event loop having been pumped (it has not been — a delivery count // would read 0 here whether or not replies existed). - // * all 800 waiters are registered. A call that completed during the loop - // would have been reaped by one of the spawns behind it, so this number - // falling short is the ungated behaviour coming back. + // * all 800 calls are registered in CallState::inflight. A call that + // completed during the loop would have left it, so this number falling + // short is the ungated behaviour coming back. It is also the fold's own + // headline, measured: 800 concurrent async calls, zero threads. const int answeredAtIssueEnd = host.answered(); - const size_t inflight = waiterCount(plain); + const size_t inflight = inflightCount(plain); std::cout << " burst issued: provider replies=" << answeredAtIssueEnd - << " waiters registered=" << inflight << "/" << kBurst << std::endl; + << " calls in flight=" << inflight << "/" << kBurst << std::endl; ASSERT_EQ(answeredAtIssueEnd, 0) << "the gate leaked: replies were produced while the burst was still " "being issued, so what this test measures below is not a burst drain"; @@ -717,9 +737,9 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) "the burst finished issuing"; // Drain it, kRelease at a time and NEVER issuing another call — which is the - // claim. Every entry that leaves m_waiters from here leaves by the exit - // guard: the spawn-path reaper has already run 800 times against an empty - // publish list and will not run again. + // claim. Every entry that leaves CallState::inflight from here leaves + // because its own call was delivered; nothing else in the object touches + // that map while the handle is alive. for (int done = 0; done < kBurst; done += kRelease) { const int upto = std::min(done + kRelease, kBurst); host.allow(upto); @@ -751,40 +771,36 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) QThread::msleep(10); } - const size_t idle = waiterCount(plain); - std::cout << " " << kBurst << " completed calls then IDLE -> m_waiters=" + const size_t idle = inflightCount(plain); + std::cout << " " << kBurst << " completed calls then IDLE -> in-flight=" << idle << std::endl; EXPECT_EQ(d.worst(), 1); EXPECT_EQ(d.missing(), 0); EXPECT_EQ(d.errors.load(), 0); - // kDrained is 64 and it is a CONSTANT again, which the ungated version could - // not afford. It is 4 * kRelease rather than a fraction of kBurst because - // that is what the quantity is: the residue is what published after the last - // reap, the last reap is inside the last release step, and so the residue is - // bounded by a step and not by the burst. It does not grow with kBurst and it - // does not grow with load. + // kDrained is 8 and it is a CONSTANT, which the ungated version of this test + // could not afford (its residue was a draw from the scheduler; see the + // header). Here the measured value is not "small", it is ZERO — a delivered + // call has already left the registry — so 8 is not headroom over a + // distribution, it is slack for a shape this branch does not currently have: + // an entry whose erase is deferred to a later turn of the loop. // - // MEASURED, correct code, 380 runs, worst value per cell: + // MEASURED, this code, 380 runs, worst value per cell: // - // macOS idle 1 (n=100) 4x 1 (n=40) 16x 2 (n=40) - // Linux idle 7 (n=40) 4x 5 (n=60) 16x 6 (n=60) 64x 5 (n=40) + // macOS idle 0 (n=100) 4x 0 (n=40) 16x 0 (n=40) + // Linux idle 0 (n=40) 4x 0 (n=60) 16x 0 (n=60) 64x 0 (n=40) // - // The same seven cells with the exit-guard reap removed, 380 more runs, give - // 800 — kBurst, not "several hundred" — in every single one, on both - // platforms, at every load level: with no reap on the exit path and no - // further spawn there is no code left that can remove an entry. All 380 of - // those runs FAILED this assertion and all 380 correct-code runs passed it. + // The same seven cells with `st->inflight.erase(id)` removed from + // AsyncCall::deliver(), 380 more runs, give 800 in every single one, on both + // platforms, at every load level. All 380 of those runs FAILED this + // assertion and all 380 unmodified runs passed it. // - // MARGINS: 9.1x above the worst correct-code value ever seen (7, Linux - // idle), 12.5x below the defect's 800. There is no overlap anywhere to - // report — the two arms are 114x apart at their closest and the defect's - // side is not a distribution at all. The load levels do not move the - // correct-code side either, which is the point of pacing the drain: the - // worst value on Linux came from the UNLOADED cell. + // MARGINS: the correct-code side never leaves the floor, so the headroom is + // 8 over 0, and 100x below the defect's 800. There is no overlap to report + // and neither side is a distribution. EXPECT_LE(idle, kDrained) << "a burst that went idle left " << idle << " of " << kBurst - << " waiters parked: they are only being reaped on the spawn path"; + << " calls registered: completed calls are not leaving the registry"; // And the handle still works afterwards — draining from inside the waiters // must not have disturbed the object they are draining. @@ -797,12 +813,10 @@ TEST_F(PlainWaiterReapingTest, BurstThatGoesIdleDrainsWithoutAnotherCall) pumpUntilTotal(after, 1, 10000); EXPECT_EQ(after.total.load(), 1); EXPECT_EQ(after.errors.load(), 0); - // Same claim, same bound — but this one also had a spawn to help it, so it - // lands at 1-2 in practice. It is held to the same expression on purpose: a - // residue that the follow-up call did NOT collect is the same retention bug, - // and hard-coding a tighter number here would put the magic constant back in - // a quieter place. - EXPECT_LE(waiterCount(plain), kDrained); + // Same claim, same bound, read once more after a further call has been and + // gone: 0 in practice, and 801 with the erase removed — the retention grows + // by exactly the one call, which is the shape of the bug this pins. + EXPECT_LE(inflightCount(plain), kDrained); // release() + pump: see ReleaseOnExit above. It runs on every exit path from // here, not just this one. @@ -868,13 +882,13 @@ TEST_F(PlainWaiterReapingTest, ReapingRacesPublishingWithoutDeadlocking) const qint64 elapsed = total.elapsed(); std::cout << " " << kCalls << " calls across " << kRounds - << " bursts in " << elapsed << "ms, m_waiters=" - << waiterCount(plain) << std::endl; + << " bursts in " << elapsed << "ms, in-flight=" + << inflightCount(plain) << std::endl; EXPECT_EQ(d.total.load(), kCalls) << "callbacks went missing under the race"; EXPECT_EQ(d.worst(), 1) << "a callback fired more than once under the race"; EXPECT_EQ(d.missing(), 0); - EXPECT_LE(waiterCount(plain), size_t(4 * kPerRound)) + EXPECT_LE(inflightCount(plain), size_t(4 * kPerRound)) << "the registry grew across the bursts"; obj->release(); @@ -912,7 +926,7 @@ TEST_F(PlainWaiterReapingTest, TeardownAfterReapingStillJoinsAndDeliversOnce) pumpUntilTotal(warm, i + 1, 10000); } ASSERT_EQ(warm.total.load(), kWarm); - ASSERT_LE(waiterCount(plain), 8u) << "the warmup calls were not reaped"; + ASSERT_LE(inflightCount(plain), 8u) << "the warmup calls were not reaped"; // Now release with a call that REALLY is outstanding: `block` parks in the // provider until the host is torn down, so the waiter is unambiguously