mirror of
https://github.com/logos-co/logos-protocol.git
synced 2026-08-30 21:41:10 +00:00
master
1
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dario LipicarandClaude Opus 5
|
01ebf33c8a |
fix(plain): answer a call that registers as the connection fails, instead of leaving it to its deadline (#49)
* fix(plain): answer a call that registers as the connection fails, instead of leaving it to its deadline
sendCallAsync() reads m_stopped and THEN registers its handler under m_mu.
fail() writes m_stopped and THEN sweeps the pending map under the same mutex.
The two are ordered opposite ways round, so a fail() that completes in between
sweeps a map the caller has not written to yet:
caller fail()
------------------------------ -------------------------------
m_stopped.load() -> false
CAS m_stopped -> true
lock(m_mu); swap(m_pendingCalls)
unlock(m_mu) ... the map was EMPTY
lock(m_mu); m_pendingCalls[id] = h
writeFrame() ... drops: stopped
The handler is now parked in the pending map of a connection nobody will sweep
again — fail() runs once and has been, no reply can arrive on a closed socket,
and the frame was never written. THE CALL IS ANSWERED BY NOTHING, and what
answers instead is the caller's own deadline: callMethodAsyncWithError reports
"timeout" after the full timeoutMs, callMethodWithError blocks its thread for
the same span and reports the same wrong code, and getMethods() waits out a
hard-coded five seconds that no caller can shorten. A connection already known
to be gone is reported as a peer that was merely slow — which is also the code
callers retry and re-acquire on.
This predates the io_context fold: master has the identical shape on the
promise-based path. #46's cancelPending() only made the orphaned entry
self-cleaning rather than permanent.
THE FIX: register first, then re-read m_stopped, and reclaim our own entry if
the connection died in between. It closes the hole by an ordering argument
rather than by a smaller window:
* if fail()'s sweep ran BEFORE the registration then its CAS ran before that,
so the re-read cannot see false, and the reclaim answers the call;
* if the re-read DOES see false then, in the total order over m_stopped, it
precedes fail()'s store; the registration is sequenced-before the re-read,
so it precedes fail()'s lock, and the sweep is guaranteed to find the entry.
There is no third case, and exactly one of the reclaim and the sweep can extract
the handler because both extract-and-erase under m_mu — the same single-winner
rule dispatchIncoming and cancelPending already play by, with one more
contender. sendMethods() gets the same treatment for the same reason.
REJECTED, since the tempting fixes deadlock: holding m_mu across the check AND
the delivery self-deadlocks on the first inline delivery, because a handler here
is AsyncCall's, which calls deliver(), which calls cancelPending(), which takes
m_mu — and m_mu is not recursive (the symmetric version, fail() invoking swept
handlers under the lock, dies the same way). Moving fail()'s once-only CAS under
m_mu is correct and deadlock-free, but makes teardown's flag wait on a mutex
every in-flight send and every decoded reply also take, so m_stopped stops being
the instantly-visible "stop writing" signal that writeFrame(), doWrite() and
doRead() read lock-free — a race traded for a teardown-latency regression.
sendSubscribe() has the same shape and is deliberately left alone, with a note
saying why: nobody waits on a subscription, so there is no deadline to blow and
no caller to strand.
tests/protocol/test_plain_send_after_fail.cpp builds the interleaving instead of
waiting for it — it takes the connection's own mutex, which parks the caller
between its m_stopped check and its registration, then drops the mutex and calls
stop() from the hot thread. Validated against
|