lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(xce-test-cli-ux): add per-connection lock to interruptible-call adapter

Browse source 
A registry of per-connection locks, acquired (interruptibly) before
spawning a worker and released by whichever side finishes last, so an
abandoned call's worker is never running concurrently with a new call
on the same connection. Uses a POSIX semaphore (sem_t), not a mutex:
review round 1 found the initial pthread_mutex_t implementation relied
on behavior POSIX classifies as undefined for a non-owning-thread
unlock (not just error-checking mutex types) -- this design's worker
thread legitimately releases a lock the caller thread acquired, which
sem_t supports with no ownership concept at all. bug-1791. Design.md
synced to reflect the semaphore choice. 1285/1285 controller suite
passing.

Task 1.5 of .kiro/specs/xce-test-cli-ux/tasks.md.
Requirements: 1.1, 1.5

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019jaMf9QEwpugH2W9KAuc7f
This commit is contained in:
Rongsong Shen 2026-09-09 09:36:27 +00:00
parent 9a06047da2
commit 204f81d134
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
6 changed files with 1226 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

181
src/controller/testsuite/interruptcall.h
View file

@ -34,15 +34,82 @@
* pthread_cancel() was rejected (cancellation-safety of
* pthread_cond_timedwait()'s internal state is not established).
*
* NOTE ON SCOPE (task 1.4 boundary): this module has NO notion of
* "connection identity" at all -- it does not know or care what `ctx`
* represents, and does not serialize two calls against each other in any
* way. The per-connection lock registry that serializes overlapping calls
* sharing a connection (including a call issued right after a prior one
* on that connection was abandoned) is task 1.5's addition, layered on
* top of this exact xcetest_run_interruptible() signature -- deliberately
* unchanged by task 1.5 so it can add that behavior without this module
* needing to guess its shape.
* PER-CONNECTION LOCK REGISTRY (task 1.5, Requirements 1.1, 1.5): an
* abandoned worker thread from task 1.4's mechanism above keeps running
* against whatever connection state `ctx` closes over. If the operator's
* very next command reuses that SAME connection, its own call would race
* the still-running abandoned worker for that connection's single-
* outstanding-reply state -- not a rare edge case, but the direct, likely
* consequence of Ctrl-C's own purpose (cancel, then immediately try
* something else). xcetest_run_interruptible() closes this by acquiring
* a per-connection lock -- looked up or lazily created in a small,
* process-lifetime registry keyed by whatever `xcetest_set_active_
* connection()` was last called with -- BEFORE spawning the worker, and
* holding it for the call's full lifetime, including through an
* abandonment (ownership of "release the lock" passes to the worker in
* that case, exactly mirroring the heap-block CAS handoff above). Lock
* acquisition is itself interruptible (the same break_out poll loop), so
* a second call waiting on a connection a prior abandoned call still
* holds can itself be Ctrl-C'd out of that wait, before ever acquiring
* the lock or spawning a worker of its own. See design.md's "Interruptible
* RPC Call and Ctrl-C Abandonment" sequence diagram and research.md's
* "Decision: Per-connection lock serializes overlapping calls on the
* Interruptible Call Adapter" for the full rationale.
*
* WHY THE LOCK IS A POSIX SEMAPHORE (sem_t), NOT A MUTEX: the handoff
* above is a genuine CROSS-THREAD release -- the REPL/caller thread
* acquires the lock before spawning the worker, but on the abandonment
* path it is the still-running WORKER thread that later releases it,
* once fn() finishes. A pthread_mutex_t (of ANY type, including the
* default PTHREAD_MUTEX_NORMAL) is the wrong primitive for this: POSIX
* defines pthread_mutex_t in terms of a single owning thread, and
* classifies pthread_mutex_unlock() from a non-owning thread as
* undefined behavior for every mutex type -- PTHREAD_MUTEX_ERRORCHECK/
* _RECURSIVE are simply *required* to detect and report that UB (hence
* the EPERM an earlier version of this code hit immediately), while
* NORMAL/DEFAULT are merely *permitted* not to detect it. POSIX does NOT
* separately declare cross-thread unlock well-defined for NORMAL; it
* just does not mandate an error return for it. A pthread_mutex_t-based
* fix that swaps ERRORCHECK for the default type therefore only trades a
* loud crash for a silent reliance on non-portable, POSIX-classified-UB
* behavior -- something that happens to hold on this project's actual
* target (glibc/NPTL's PTHREAD_MUTEX_NORMAL has had no ownership
* tracking for decades, and XCE's Nix-hermetic build is glibc/Linux-only)
* but is the wrong thing to build on when a fully-defined alternative
* exists at the same code complexity.
*
* sem_t has NO ownership concept at all: sem_wait()/sem_trywait()/
* sem_post() may be called from any thread regardless of which thread
* called which, with fully-defined POSIX semantics for exactly this
* cross-thread signal pattern. Each registry entry is therefore a binary
* semaphore (sem_init(&sem, 0, 1)) used as a mutual-exclusion lock:
* sem_trywait() takes the place of pthread_mutex_trylock() in the
* interruptible poll loop below (EAGAIN where trylock would have
* returned EBUSY), and sem_post() takes the place of
* pthread_mutex_unlock() at both release sites -- with no UB caveat on
* either.
*
* WHY THE REGISTRY IS A SINGLY-LINKED LIST, NOT A GROWABLE ARRAY: a
* sem_t* handed out by this registry may be cached inside a call block
* that a still-running abandoned worker thread holds onto indefinitely
* (up to ~10s, per the heap-block ownership handoff above). A growable
* array's realloc() can MOVE every previously-returned element the
* moment a later, unrelated connection is first seen -- silently
* invalidating that worker's pointer into stale/reused memory. A linked
* list's nodes are individually allocated and, per this registry's own
* "entries are never removed" contract, never freed or moved for the
* rest of the process's life, so a pointer into it stays valid forever
* once handed out.
*
* CONCURRENCY OF THE REGISTRY ITSELF: per design.md's Implementation
* Notes, "the registry's lookup/insert step needs no synchronization of
* its own -- a new registry entry is only ever created from the single
* REPL thread that issues commands" (detached worker threads never
* initiate a new top-level call; they only run the `fn` they were given
* and release the lock/free the block they were handed). The SEMAPHORE
* each entry owns, once handed out, is of course safe for concurrent
* wait/post from different threads -- that safety is sem_t's own job,
* not the registry's.
*/
#include <stdatomic.h>
@ -74,6 +141,42 @@ typedef enum {
XCT_ABANDONED
} xcetest_call_state_t;
/*
* Sets the connection-identity key that xcetest_run_interruptible() below
* reads INTERNALLY when a new call starts, to look up (or lazily create)
* that connection's entry in the per-connection lock registry (task 1.5,
* see the file header above). This is a simple settable-elsewhere,
* read-internally piece of process state -- deliberately mirroring how
* break_out/xcetest_interrupted() already works -- NOT a parameter added
* to xcetest_run_interruptible()'s own signature, which stays exactly
* `(fn, ctx)` per this task's design constraint.
*
* SEAM FOR A FUTURE CALLER (task 3.2): production code (test.c's cmd_*
* functions) does not exist inside this module and has no notion of
* "the currently active command_ctx_t's connection pointer" that this
* header can reach into directly -- test.c itself is never linked into
* this binary's unit tests (see this file's own header and
* xcetest_interrupt.h's). The intended real wiring is: before invoking
* xcetest_run_interruptible() for a blocking RPC, a cmd_* function (or a
* shared dispatch point above all of them) calls
* xcetest_set_active_connection(<that connection's own stable pointer,
* e.g. the command_ctx_t's connection field>). Any stable, comparable-by-
* identity pointer works as `conn_key` -- this module never dereferences
* it, only compares it by pointer equality against the registry's keys.
* Until task 3.2 wires that in, xce_test's real binary calls this
* exactly once, from wherever main()/the dispatcher sets up each
* command's active connection context.
*
* Concurrency: like break_out, callable only from the single REPL thread
* that issues top-level commands -- see the file header's "CONCURRENCY
* OF THE REGISTRY ITSELF" note. Passing the same key across unrelated
* connections would incorrectly serialize them against each other;
* passing a fresh/different key per connection is what keeps unrelated
* connections independent (Requirement 1.5, "leave ... its underlying
* connection intact").
*/
void xcetest_set_active_connection(void* conn_key);
/*
* Runs `fn(ctx)` on a detached worker thread while THIS (the calling)
* thread polls xcetest_interrupted() every ~50ms (design.md's Interruptible
@ -81,6 +184,16 @@ typedef enum {
* it immediately instead of blocking for fn()'s own, possibly much
* longer, duration.
*
* Before spawning that worker, this function first acquires -- also
* interruptibly, via the same ~50ms xcetest_interrupted() poll, but
* using sem_trywait() instead of a blocking sem_wait() -- the
* per-connection lock for whichever connection key was last passed to
* xcetest_set_active_connection() (task 1.5; see the file header's "PER-
* CONNECTION LOCK REGISTRY" section above). This serializes this call
* against any other in-flight or abandoned-but-still-running call on the
* same connection, so that connection's single-outstanding-reply state
* is never touched by two threads at once.
*
* Preconditions:
* - `fn` is non-NULL.
* - `fn` touches only what `ctx` itself heap-owns -- NEVER memory on
@ -93,18 +206,32 @@ typedef enum {
* - Returns fn()'s own return value when fn() finishes before this
* thread's abandon-CAS wins the race (whether or not
* xcetest_interrupted() ever became true during the wait) -- see the
* header comment's "near-simultaneous" case below.
* - Returns -EINTR when this thread observes an interrupt and its own
* CAS (RUNNING -> ABANDONED) wins the race. On this return, the
* caller MUST NOT touch `ctx` again -- ownership has passed to the
* still-running worker thread, which will finish fn() and free the
* call block on its own, asynchronously, with no further signal back
* header comment's "near-simultaneous" case below. On this path, the
* per-connection lock is released by this (the caller's) thread,
* after reading rc and freeing the block.
* - Returns -EINTR when this thread observes an interrupt WHILE
* WAITING FOR THE PER-CONNECTION LOCK -- i.e. before ever acquiring
* it. No worker is spawned for this call at all in this case, and
* nothing needs releasing, since this call never held the lock to
* begin with.
* - Returns -EINTR when this thread observes an interrupt AFTER
* acquiring the lock and spawning the worker, and its own CAS
* (RUNNING -> ABANDONED) wins the race. On this return, the caller
* MUST NOT touch `ctx` again -- ownership of both the call block AND
* the per-connection lock has passed to the still-running worker
* thread, which will finish fn(), free the call block, and release
* the lock on its own, asynchronously, with no further signal back
* to this caller.
* - Returns -EINVAL if `fn` is NULL (fn is never invoked in this case).
* - Returns a negative errno from pthread_attr_init()/pthread_create()
* if the worker thread could not be set up or spawned at all; the
* call block is freed before returning and fn() is never invoked in
* this case either.
* - Returns -EINVAL if `fn` is NULL (fn is never invoked in this case;
* no lock is acquired).
* - Returns a negative errno from the per-connection lock registry
* lookup (e.g. -ENOMEM), from sem_trywait() itself for an
* unexpected error other than EAGAIN, or from pthread_attr_init()/
* pthread_create() if the worker thread could not be set up or
* spawned at all; in every one of these cases fn() is never invoked,
* any call block already allocated is freed before returning, and
* the per-connection lock -- if this call had already acquired it --
* is released before returning.
*
* The near-simultaneous race (this thread notices the interrupt in the
* same window the worker finishes fn()) is resolved by the SAME single
@ -113,14 +240,12 @@ typedef enum {
* attempt} executes first wins, and the loser observes that the state is
* no longer RUNNING and falls back to the OTHER side's outcome -- a
* caller who loses the abandon-CAS falls through to the normal
* completion path (reads rc, frees the block, returns rc) instead of
* returning -EINTR; a worker who loses the done-CAS frees the block
* itself instead of leaving it for the caller. Exactly one free() of the
* block is therefore reachable on every possible interleaving -- never
* zero, never two.
*
* Does not acquire any lock and has no notion of connection identity --
* see the file header's "NOTE ON SCOPE" above.
* completion path (reads rc, frees the block, releases the lock, returns
* rc) instead of returning -EINTR; a worker who loses the done-CAS frees
* the block and releases the lock itself instead of leaving them for the
* caller. Exactly one free() of the block, and exactly one release of
* the lock, are therefore reachable on every possible interleaving --
* never zero, never two.
*/
int xcetest_run_interruptible(xcetest_blocking_fn fn, void* ctx);