mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-30 13:31:12 +00:00
* fix(plain): give every handle its own event subscription on the shared connection
RpcConnection::m_eventCallbacks was keyed by (object, eventName) and ASSIGNED.
One RpcConnection is shared by every PlainLogosObject a PlainTransportConnection
hands out, and requestObject() mints a fresh handle per acquire, so the second
handle to subscribe to the same event on the same module silently took the first
one's channel — including the deferred ("multi") completion channel every handle
subscribes to on its first call.
No concurrency is needed to reach it. Measured on cf1b9b0: handle A's deferred
call answers 8/8 in 0 ms alone, and 0/8 in 1505 ms (timeout, INVALID) once a
second handle merely exists. Identical on master.
THE WIRE DOES NOT MOVE. Subscribe/Unsubscribe still carry (object, event) and
nothing else. The host keeps ONE sink per (object, event, connection) — the right
model, since every sink for a connection is the same "write this frame back down
that socket" — and the CONSUMER, the only end that knows how many of its own
handles want an event, does the demultiplexing: a list of registrations per key,
a subscription id to withdraw one of them, and an Unsubscribe frame only when the
last local registration for the pair is gone, because that frame is
connection-wide.
Also here, because the consumer-side fan-out makes it observable as a doubled
delivery: PlainTransportHost::fanOutEvent sent a connection subscribed both by
name and by wildcard two copies of the same event. One copy per connection now.
And the parked-completion staging area is gated. It exists for one ordering — a
"multi" worker that finishes before the sentinel it answers has been written, so
the completion event overtakes its own Result — and a completion arriving that
early is not attributable to a handle at all. Every handle now sees every
completion on the object, so parking is allowed only while this handle has a call
outstanding, the map is emptied the instant none is, and it is capped at 512 with
oldest-first eviction. Pre-fix the same leak existed with one victim instead of N:
the handle that had stolen the channel parked every other handle's completions and
never claimed one.
Detectors: nine tests in test_plain_event_sub_sharing.cpp, all validated RED on
cf1b9b0 and on master and green here, including both mixed-version directions (a
new consumer against a verbatim pre-fix host, and an old consumer's frame sequence
against the new host over a raw socket).
── REBASED ONTO #50, THE FIRST REPLAY IN THIS STACK WITH A REAL PRODUCTION
CONFLICT (and then onto #51, which lands cleanly) ──────────────────────────
the object (a live-reference count with deferred destruction) and to give async
callbacks somewhere to land in a Qt-free process (a never-destroyed
DeliveryService). Both of those and both of this commit's changes are kept; the
resolution picks no side anywhere.
* plain_logos_object.cpp, hunk 1 — #50 replaced postToQtEventLoop's comment
header with DeliveryService; this commit inserted SyncCallScope immediately
above it. The insert point survives, the replaced header does not:
SyncCallScope now sits above #50's DeliveryService block.
* plain_logos_object.cpp, hunk 2 — #50 split disconnectEvents() into a guarded
entry point plus an unguarded disconnectEventsImpl() (release() and the
destructor must not take a reference to what they are destroying); this
commit changed that body's local from (name, callback) pairs to subscription
ids. Kept as #50's split with this commit's body. onEvent() auto-merged the
same way: #50's EntryGuard declared FIRST, then the m_mu-across-the-subscribe
body from here.
* test_sync_call_release_race.cpp — not a text conflict and not visible to git:
#50's StalledConnection double implements RpcConnectionBase, and this commit
changes that interface, so it became abstract and the file stopped compiling.
Its stub now returns a DISTINCT id per subscribe, so it cannot hide a bug
that withdraws the wrong registration.
* tests/protocol/CMakeLists.txt — both detector lists are additive and both are
in. The prose is NOT concatenated: an earlier replay on this stack had
grafted a duplicated fragment ("per-path test as evidence the gate is there.
Nor is") into the middle of a paragraph, and the sentence it belonged to is
now where it was meant to go, at the end of the "what is NOT on that list"
paragraph. #50's "the CAS is TWO gates" correction is untouched. #51's own
additions to the same file merged without a conflict on the second replay.
gives the five LiveHost fixtures a teardown that destroys the host on the thread
that emits into it, and the host in test_plain_event_sub_sharing.cpp is not that
shape — its ModuleProxy stays on the test thread, so there is no worker to race.
RE-VERIFIED BY RUNNING, on macOS arm64, Debug, all after the rebase:
* THIS FIX still fixes the bug on the NEW master. The ten
PlainEventSubSharingTest cases compile unmodified on 5be3a84 and nine go RED
there: handle A alone 8/8 answered at 0 ms avg, handle A once B exists 0/8 at
1503 ms avg with 8 timeouts, and 0/4 with 4 timeouts through the shipping
host. On this commit: 8/8 and 4/4, 0 ms avg, 0 timeouts. So neither #50 nor
#51 caused or masked this defect, and this branch is not a no-op.
* #50's FIX still works through this change. SyncCallReleaseRaceTest 8/8 green
(release() returns in 0 ms with a call parked, destroyed=1 only after the
caller leaves; 400 release-wake races, one destruction each), the whole no-Qt
binary 7/7 plus the after-main() probe (delivered=1, off the issuing thread,
exit 0), and its mechanisms are still in the source: m_liveRefs with the
EntryGuard reference-first/reference-last ordering, release() dropping the
owner's reference instead of `delete this`, DeliveryService `new`-ed with
`~DeliveryService() = delete` and a detached thread, and NO m_conn.reset() in
release().
* EXACTLY-ONCE, in the release-race shape rather than the per-path pins. Qt
vehicle: 20 rounds x 500 calls released mid-burst, answered-by-reply=1034,
cancelled-by-teardown=8966, 0 doubles, 0 dropped. No-Qt twin: 1130 / 8870, 0
and 0. Both resolvers live in both. Re-validated as a DETECTOR on this merged
tree by removing BOTH gates (claim()'s CAS and takeCallback()'s swap) in a
throwaway build: 19 doubles Qt, 13 doubles no-Qt, both FAIL, while the
per-path exactly-once tests stay green — which is the difference the
CMakeLists note describes.
* NO USE-AFTER-FREE. Guard Malloc clean over SyncCallReleaseRace, IoFold,
PlainObjectTeardown, PlainCompletionSubLifetime, PlainCancelPendingRace,
PlainWaiterReaping, PlainHostEventTeardown, PlainEventSubSharing and
PlainParkedCompletionGate (52/52), and over the whole no-Qt binary (7/7,
exit 0).
* TEARDOWN AND DEADLINES. release() with 32 calls in flight: 0 ms. release()
after 100 reaped calls: 0 ms. A 200 ms deadline fires at 200 ms while an
onEvent handler holds the io thread for 2001 ms; a 250 ms deadline fires at
250 ms with the io thread blocked forever; 12 idle 200 ms deadlines
min=200 median=200 max=202 ms.
* NO USER CALLBACK INLINE ON AN IO THREAD: no-Qt replies 300/300 with
on-caller-thread=0 and delivery-thread!=io-thread=1; cancellations 20/20 with
inside-release=0 and on-releasing-thread=0.
* Full suite 433/433 (was 421 on master; this adds 12), and `nix build .#tests`
433/433.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(plain): make the subscription detectors barrier-driven instead of timed
Every "wait a bit then count" in the new tests is now an in-band round trip: a
Methods request travels the same socket and is dispatched on the same strand as
everything written before it, so its reply proves the earlier Subscribe /
Unsubscribe frames have been applied AND that the Event frames the peer wrote
before answering have been dispatched. The audits are exact counts rather than
polls with a timeout, which is what they have to be on a loaded CI runner — a
short sleep there fails the test rather than skipping it.
Still 9 of 10 RED on cf1b9b0 and on master, with the teardown pin green; the set
now runs in 9s instead of 110s.
* test(plain): drop the poll helper the barrier replaced
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
74 lines
3.4 KiB
C++
74 lines
3.4 KiB
C++
#ifndef LOGOS_PLAIN_INCOMING_CALL_HANDLER_H
|
|
#define LOGOS_PLAIN_INCOMING_CALL_HANDLER_H
|
|
|
|
#include "rpc_message.h"
|
|
|
|
#include <functional>
|
|
|
|
namespace logos::plain {
|
|
|
|
// -----------------------------------------------------------------------------
|
|
// IncomingCallHandler — provider-side dispatch hook.
|
|
//
|
|
// rpc_connection hands inbound Call / Methods / Subscribe / Unsubscribe /
|
|
// Token messages to a handler that the Qt-boundary layer implements. The
|
|
// handler is what talks to the published QObject (ModuleProxy); this
|
|
// interface deliberately speaks only plain C++ types so the wire stack
|
|
// stays Qt-free.
|
|
//
|
|
// The reply callbacks can be invoked synchronously (from inside the
|
|
// handler) or asynchronously from a different thread — rpc_connection
|
|
// serializes the actual frame write internally.
|
|
// -----------------------------------------------------------------------------
|
|
class IncomingCallHandler {
|
|
public:
|
|
virtual ~IncomingCallHandler() = default;
|
|
|
|
using CallReply = std::function<void(ResultMessage)>;
|
|
using MethodsReply = std::function<void(MethodsResultMessage)>;
|
|
using EventSink = std::function<void(EventMessage)>;
|
|
|
|
virtual void onCall(const CallMessage& req, CallReply reply) = 0;
|
|
|
|
virtual void onMethods(const MethodsMessage& req, MethodsReply reply) = 0;
|
|
|
|
// `sink` stays alive until onUnsubscribe fires or the connection
|
|
// dies. The handler must call `sink(evt)` on every matching emission.
|
|
//
|
|
// `connectionId` is an opaque per-connection token (the rpc layer
|
|
// passes the connection's `this` pointer). The handler keys sinks
|
|
// by it so a subsequent onUnsubscribe / onConnectionClosed can
|
|
// remove only the sinks belonging to that connection — sub/unsub
|
|
// frames don't carry a subscriber identifier on the wire.
|
|
//
|
|
// ONE SINK PER (object, event, connection), AND THAT IS THE CONTRACT, not a
|
|
// simplification waiting to be lifted. Every sink a handler could build for a
|
|
// given connection is the same thing — "write this frame back down that
|
|
// socket" — so a second Subscribe for a pair this connection already has is
|
|
// an idempotent re-assertion, and a handler is right to overwrite. It follows
|
|
// that a CONSUMER with several logical subscribers behind one connection owns
|
|
// the demultiplexing: it fans one delivery out locally (see
|
|
// RpcConnection::sendSubscribe) and sends Unsubscribe only when the last of
|
|
// them is gone, because Unsubscribe means "this connection wants no more of
|
|
// that event at all". A consumer that unsubscribes per-subscriber silences
|
|
// its own siblings, and no host-side bookkeeping can tell that apart from a
|
|
// genuine unsubscribe.
|
|
virtual void onSubscribe(const SubscribeMessage& req, EventSink sink,
|
|
const void* connectionId) = 0;
|
|
|
|
virtual void onUnsubscribe(const UnsubscribeMessage& req,
|
|
const void* connectionId) = 0;
|
|
|
|
// Called when a connection is torn down (graceful close or error)
|
|
// so the handler can drop any sinks still keyed to it. Without
|
|
// this, a dropped client leaks subscriptions and the host keeps
|
|
// fanning events into dead sinks.
|
|
virtual void onConnectionClosed(const void* connectionId) = 0;
|
|
|
|
virtual void onToken(const TokenMessage& req) = 0;
|
|
};
|
|
|
|
} // namespace logos::plain
|
|
|
|
#endif // LOGOS_PLAIN_INCOMING_CALL_HANDLER_H
|