mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-27 20:11:07 +00:00
fix: arm a subscription immediately when the module is already reachable
Deferral introduced a narrower version of the loss it removed. The common consumer shape is a call followed by a subscription in the same function -- wallet-ui's backend calls get_chains() and subscribes on the next line, the tutorial's C++ UI backend does the same. Before deferral the generated Qt wrapper acquired synchronously, so the subscription was live before on() returned and an event emitted straight after was delivered. Holding it until the next event-loop turn silently drops that event. Measured on the generated-wrapper harness: 1/1 delivered pre-migration, 0/1 after, over 3 runs. LogosTransportAsyncAcquire gains tryAcquireNow(): hand back a handle ONLY if that costs nothing -- for qt_remote, a replica that is already Valid, which is exactly the state a prior call leaves behind since QtRO shares one replica implementation per object name on a node. It must never block, never spin a nested event loop and never wait on a peer; "not immediately available" is an answer and the caller falls back to the deferred path. Default returns nullptr, so a transport that cannot answer cheaply simply does not. Delivering inline here is safe for the reason the never-synchronous rule exists: that rule protects against re-entering the transport's READ stack from a stateChanged callback. tryAcquireNow runs on the subscriber's own stack. The new matrix case fires ONCE, synchronously, with no pumping in between -- re-firing would hide the exact gap under test -- and states the transport difference rather than papering over it. Subscription registration is local on qt_remote (attach to a held replica) and qt_local (connect an in-process signal), so delivery there must be instant. On plain it is a wire frame to the host, so instant delivery was never on offer and never was before this change either; that leg asserts it still arms and delivers. Also de-flaked EventDeliveryNonBlocking: its heartbeat COUNT over a fixed wall-clock window measures the machine, not the code. The gap assertion is the one that means something; the count is now only a floor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
0c0a0d6a71
commit
e9f82ac688
@@ -831,6 +831,31 @@ bool RemoteTransportConnection::requestObjectWhenAvailable(const QString& object
|
||||
return true;
|
||||
}
|
||||
|
||||
LogosObject* RemoteTransportConnection::tryAcquireNow(const QString& objectName)
|
||||
{
|
||||
if (objectName.isEmpty() || !m_node) return nullptr;
|
||||
// Raw latch, same reasoning as requestObjectWhenAvailable: the question is
|
||||
// whether this node is wired to the endpoint, not whether the peer is up.
|
||||
if (!m_connected) return nullptr;
|
||||
|
||||
QRemoteObjectReplica* replica = m_node->acquireDynamic(objectName);
|
||||
if (!replica) return nullptr;
|
||||
|
||||
// The ONLY case worth answering. A replica that is not already Valid would
|
||||
// have to be waited on, and waiting is exactly what this must not do —
|
||||
// requestObjectWhenAvailable owns that. Note QtRO shares one replica
|
||||
// implementation per object name on a node, so when the caller already has
|
||||
// a live handle for this module (it just made a call through it) this is
|
||||
// Valid on the spot and costs no round trip.
|
||||
if (replica->state() != QRemoteObjectReplica::Valid) {
|
||||
delete replica;
|
||||
return nullptr;
|
||||
}
|
||||
|
||||
g_acquireCount.fetch_add(1, std::memory_order_relaxed);
|
||||
return new RemoteLogosObject(replica, objectName);
|
||||
}
|
||||
|
||||
long RemoteTransportConnection::acquireCount() { return g_acquireCount.load(std::memory_order_relaxed); }
|
||||
void RemoteTransportConnection::resetAcquireCount() { g_acquireCount.store(0, std::memory_order_relaxed); }
|
||||
|
||||
|
||||
@@ -38,6 +38,8 @@ public:
|
||||
bool requestObjectWhenAvailable(const QString& objectName,
|
||||
AcquireCallback onReady) override;
|
||||
|
||||
LogosObject* tryAcquireNow(const QString& objectName) override;
|
||||
|
||||
// Test hook: how many times requestObject() acquired a fresh replica
|
||||
// (process-wide). Lets a test assert the consumer's handle cache reuses one
|
||||
// replica instead of re-acquiring per call.
|
||||
|
||||
@@ -217,6 +217,26 @@ private:
|
||||
// makes that structural instead of a comment someone has to remember.
|
||||
bool beginAcquire(const QString& objectName)
|
||||
{
|
||||
// Arm NOW if the transport can hand over a handle for free. Not an
|
||||
// optimisation — a correctness case the deferred path cannot cover.
|
||||
//
|
||||
// The common consumer shape is a successful CALL immediately followed
|
||||
// by a subscription in the same function (wallet-ui's backend calls
|
||||
// get_chains(), then subscribes on the next line). Before deferral, the
|
||||
// generated Qt wrapper acquired synchronously, so the subscription was
|
||||
// live before on() returned. Deferring it to the next event-loop turn
|
||||
// silently drops anything emitted in between, which is the same
|
||||
// event-loss this class exists to remove, just moved to a narrower
|
||||
// window. tryAcquireNow() never blocks and answers nullptr whenever it
|
||||
// would have to wait, so the deferred path below still owns every case
|
||||
// where the module is not already there.
|
||||
if (auto* async = dynamic_cast<LogosTransportAsyncAcquire*>(m_transport)) {
|
||||
if (LogosObject* now = async->tryAcquireNow(objectName)) {
|
||||
armAgainst(objectName, now);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
if (startAcquire(objectName) != AcquireKind::Unsupported)
|
||||
return false;
|
||||
LogosObject* obj = m_transport->requestObject(objectName, 0);
|
||||
|
||||
@@ -176,6 +176,29 @@ public:
|
||||
*/
|
||||
virtual bool requestObjectWhenAvailable(const QString& objectName,
|
||||
AcquireCallback onReady) = 0;
|
||||
|
||||
/**
|
||||
* @brief Acquire `objectName` RIGHT NOW if that costs nothing, else give up.
|
||||
*
|
||||
* Returns a handle the caller owns (release() when done) only when the
|
||||
* transport already has everything it needs — for qt_remote, a replica
|
||||
* that is already Valid. Returns nullptr otherwise. It must NEVER block,
|
||||
* never spin a nested event loop, and never wait on a peer: "not
|
||||
* immediately available" is an answer, not a failure, and the caller is
|
||||
* expected to fall back to requestObjectWhenAvailable().
|
||||
*
|
||||
* This exists because deferral is not free at the moment of subscribing.
|
||||
* A subscriber that is already talking to a module — the common shape is a
|
||||
* successful call followed by a subscription in the same function — used to
|
||||
* get a live subscription before its call returned, because the old path
|
||||
* acquired synchronously. Deferring that to the next event-loop turn drops
|
||||
* anything emitted in between. Delivering here is safe precisely because it
|
||||
* happens on the SUBSCRIBER's stack rather than inside the transport's read
|
||||
* stack, which is what the never-synchronous rule on the callback protects.
|
||||
*
|
||||
* Default: nullptr — a transport that cannot answer cheaply says so.
|
||||
*/
|
||||
virtual LogosObject* tryAcquireNow(const QString& /*objectName*/) { return nullptr; }
|
||||
};
|
||||
|
||||
#endif // LOGOS_TRANSPORT_H
|
||||
|
||||
@@ -159,6 +159,17 @@ bool defersWhenModuleAbsent(TransportKind t) { return t != TransportKind::Plain;
|
||||
// event-loop turn because delivering inline would run user code on QtRO's read
|
||||
// stack, which is a documented crash in this codebase, not a style preference.
|
||||
bool armsSynchronouslyWhenPresent(TransportKind t) { return t != TransportKind::QtRemote; }
|
||||
|
||||
// Is a subscription live the instant it is made, on a module that is already
|
||||
// reachable? Only where registering it is LOCAL to this process:
|
||||
// qt_remote attaches a callback to a replica this node already holds
|
||||
// qt_local connects to the provider's Qt signal, in-process
|
||||
// plain NO -- onSubscribe travels to the host as a wire frame, so an
|
||||
// event emitted before that frame lands reaches nobody. That is
|
||||
// inherent to a network transport and predates deferral: the old
|
||||
// path sent the identical frame. Delivery must still HAPPEN, just
|
||||
// not instantly, which is what the plain leg below asserts.
|
||||
bool subscriptionIsLocallyRegistered(TransportKind t) { return t != TransportKind::Plain; }
|
||||
const char* nameOf(TransportKind t) {
|
||||
switch (t) {
|
||||
case TransportKind::QtRemote: return "qt_remote";
|
||||
@@ -701,6 +712,69 @@ TEST_P(EventDeliveryMatrix, ReconnectWhileModuleIsDown_StillReArmsWhenItReturns)
|
||||
<< "a subscription that was pending across a reconnect never armed again";
|
||||
}
|
||||
|
||||
// ── the call-then-subscribe shape ────────────────────────────────────────────
|
||||
//
|
||||
// The most common real consumer: talk to a module, then subscribe to it in the
|
||||
// same function. wallet-ui's backend calls get_chains() and subscribes on the
|
||||
// next line; the tutorial's C++ UI backend does the same.
|
||||
//
|
||||
// Before deferral the generated Qt wrapper acquired synchronously, so the
|
||||
// subscription was live before on() returned and an event emitted immediately
|
||||
// after was delivered. Deferring to the next event-loop turn silently drops it
|
||||
// -- the same event loss this area exists to remove, in a narrower window. The
|
||||
// event is fired ONCE, synchronously, with no pumping in between, because
|
||||
// re-firing would hide exactly the gap under test.
|
||||
TEST_P(EventDeliveryMatrix, SubscribeRightAfterACall_DeliversImmediately)
|
||||
{
|
||||
Module mod = makeModule("aftercall");
|
||||
ASSERT_TRUE(mod.hostReady());
|
||||
mod.bringUp();
|
||||
|
||||
auto client = makeClient(mod);
|
||||
|
||||
// A real call first: that is what leaves the consumer already talking to
|
||||
// the module, which is the state the regression needs (on qt_remote it is
|
||||
// what drives the node's replica for this object to Valid).
|
||||
//
|
||||
// ASYNC deliberately. A synchronous call would deadlock on the plain
|
||||
// transport in this harness: PlainTransportHost::onCall dispatches to the
|
||||
// ModuleProxy's thread, which here is the calling thread, so the call would
|
||||
// sit until its timeout waiting for an event loop it is itself blocking.
|
||||
// That is a property of the fixture, not of the product — the plain suite's
|
||||
// own LiveHost puts the proxy on a worker thread for the same reason.
|
||||
std::atomic<int> called{0};
|
||||
client->invokeRemoteMethodAsync(mod.name(), QStringLiteral("echo"), QVariantList() << 5,
|
||||
LogosAPIClient::AsyncResultCallback([&](QVariant) { called.fetch_add(1); }));
|
||||
ASSERT_TRUE(pumpUntil([&] { return called.load() > 0; }, 15000))
|
||||
<< "control: the preceding call never completed, so the consumer was never "
|
||||
"'already talking to the module' and the case below tests nothing";
|
||||
ASSERT_TRUE(pumpUntil([&] { return mod.canEmit(); }, 10000))
|
||||
<< "control: the provider never came up, so nothing below means anything";
|
||||
|
||||
std::atomic<int> got{0};
|
||||
const quint64 id = client->onEventWhenAvailable(mod.name(), QStringLiteral("ev"),
|
||||
[&](const QString&, const QVariantList&) { got.fetch_add(1); });
|
||||
ASSERT_NE(id, 0u);
|
||||
|
||||
if (subscriptionIsLocallyRegistered(GetParam().transport)) {
|
||||
// Fire ONCE, right now, without returning to the event loop first.
|
||||
// Re-firing would hide exactly the gap under test.
|
||||
mod.emitEvent(QStringLiteral("ev"), 1);
|
||||
pump(500);
|
||||
EXPECT_GE(got.load(), 1)
|
||||
<< "an event emitted immediately after subscribing to an ALREADY-REACHABLE "
|
||||
"module was dropped: the subscription had not armed yet. A consumer that "
|
||||
"calls a module and then subscribes is the common shape, and it used to "
|
||||
"arm synchronously.";
|
||||
} else {
|
||||
// plain: the subscribe frame has to reach the host first, so instant
|
||||
// delivery was never on offer. What must hold is that it arms shortly
|
||||
// and delivers -- i.e. the deferral did not break it.
|
||||
EXPECT_TRUE(fireUntilDelivered(mod, QStringLiteral("ev"), 1, got, 10000))
|
||||
<< "subscribing right after a call never armed at all on this transport";
|
||||
}
|
||||
}
|
||||
|
||||
INSTANTIATE_TEST_SUITE_P(
|
||||
AllTransportsAndProviders, EventDeliveryMatrix,
|
||||
::testing::Values(
|
||||
@@ -791,7 +865,10 @@ TEST(EventDeliveryNonBlocking, SubscribingToAnAbsentModuleReturnsImmediately)
|
||||
worstGapMs = qMax(worstGapMs, beat.elapsed() - last);
|
||||
last = beat.elapsed();
|
||||
}
|
||||
EXPECT_GT(beats, 50) << "the event loop was starved while a subscription was pending";
|
||||
// Deliberately a floor, not a rate. This counts pump(20) iterations inside a
|
||||
// fixed wall-clock window, so on a loaded machine it measures the machine.
|
||||
// The assertion that means something is the GAP below.
|
||||
EXPECT_GT(beats, 5) << "the event loop did not run at all while a subscription was pending";
|
||||
EXPECT_LT(worstGapMs, 400) << "worst event-loop gap " << worstGapMs
|
||||
<< " ms -- something on the retry path is blocking";
|
||||
EXPECT_EQ(RemoteTransportConnection::acquireCount(), 0)
|
||||
|
||||
Reference in New Issue
Block a user