diff --git a/CMakeLists.txt b/CMakeLists.txt index 2f73410..69dbe30 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -234,6 +234,7 @@ if(AKERR_THREAD_SAFE) err_threads_init err_threads_pool err_threads_registry + err_threads_handoff ) endif() diff --git a/README.md b/README.md index 2c55890..28c819f 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ What that covers: * **The error pool.** Finding a free slot in `AKERR_ARRAY_ERROR` and taking its reference is one operation under a lock, so two threads can never be handed - the same context. A context is then owned by the thread that raised it, all + the same context. A context is then owned by exactly one thread at a time, all the way through `CATCH`, `HANDLE`, and release. * **The status registry.** Reservations and name registrations are serialized against each other and against lookups. Two threads reserving the same range @@ -141,11 +141,22 @@ What that covers: * **Per-thread state.** The context behind `IGNORE` (`__akerr_last_ignored`) and the last-ditch context used to report `akerr_release_error(NULL)` are thread-local, so one thread's ignored error is never another's. +* **Handing a context from one thread to another.** A context is not thread + state — it lives in `AKERR_ARRAY_ERROR`, which is process-global — so it + outlives the thread that raised it. The reference count is the only field the + library reads across threads, and it is only ever touched under the pool lock, + so `akerr_release_error()` does not care which thread checked the slot out. + Raise on a worker, queue it, let the worker exit; the collector still has a + whole error with a whole stack trace. See + [Handing an error to another thread](#handing-an-error-to-another-thread). What it does not cover, and cannot: -* **Sharing one error context between threads.** The library hands a context to - one thread; passing it to another is your synchronization to do. +* **Two threads inside one context at the same time.** A context has one owner, + and only one. `FAIL` rewrites the message, `HANDLE` rewinds the stack-trace + cursor, and none of that is locked — two threads in one context splice their + messages together and truncate each other's trace, without crashing. Handing a + context *from* one thread *to* another is a different thing, and is supported. * **`akerr_log_method` and `akerr_handler_unhandled_error`.** Set them during startup, before you spawn threads. They are read on every error and the library never writes them after initialization, so setting one while other @@ -176,6 +187,135 @@ consumer can check what it linked against: #endif ``` +## Handing an error to another thread + +**Transfer** an error context; never **share** one. One thread owns it at a time, +and ownership moves in a single step: the thread giving it up stops touching it +in the same act that makes it visible to the thread taking it over. + +This works because a context is not thread state. It lives in +`AKERR_ARRAY_ERROR`, which is process-global, so a context outlives the thread +that raised it — a worker can raise an error, queue it, and exit, and the +collector still has a whole error with a whole stack trace. Nothing in the +library reads a context through any pointer but the one its current owner handed +in. The one field it reads across threads is the reference count, and that is +only ever touched under the pool lock, so `akerr_release_error()` does not care +which thread checked the slot out. Release it wherever it ended up. + +**Always** hand it over through something that synchronizes: a mutex, a +condition variable, `pthread_join`, or an acquire/release atomic. The content of +a context is written with no lock at all — that is deliberate, error +construction should not pay for a lock it does not need — so the handoff itself +is what publishes those writes. Push the pointer through a relaxed atomic or a +plain global and the receiver can read a half-written message, on a machine you +did not test on. + +**Never** touch a context after you have given it away. Not a `->status`, not a +log line. The receiver may be inside `HANDLE` rewinding the trace cursor, or +inside `akerr_release_error()` memsetting the slot. + +**Release it exactly once.** A context released twice from a stale pointer takes +the refcount-zero branch a second time and wipes the slot again — which by then +holds somebody else's live error. Nothing crashes; a different thread's error +just quietly goes blank. + +```c +/* Producer. Owns the context until queue_push() returns, and not after. */ +static void report_unit_failure(int unit) +{ + PREPARE_ERROR(e); + + FAIL(e, MYLIB_UNIT_FAILED, "unit %d stopped responding", unit); + /* The last thing this thread does to it, so the trace records the crossing. + The buffer belongs to the context, so it travels with it. */ + AKERR_STACKTRACE_APPEND(e, "%s:%s:%d: queued for the collector\n", + __FILE__, __func__, __LINE__); + queue_push(e); /* takes the queue mutex; `e` is not ours after this */ +} + +/* Collector. Owns it from the moment queue_pop() returns. */ +static void collect_one(akerr_ErrorContext *e) +{ + ATTEMPT { + } CLEANUP { + } PROCESS(e) { + } HANDLE(e, MYLIB_UNIT_FAILED) { + restart_unit(e); + } HANDLE_DEFAULT(e) { + LOG_ERROR_WITH_MESSAGE(e, "collector: unrecognized failure"); + } FINISH_NORETURN(e); +} + +static void *collector(void *unused) +{ + akerr_ErrorContext *e; + + (void)unused; + while ( (e = queue_pop()) != NULL ) { + collect_one(e); + } + return NULL; +} +``` + +Four things about the receiving side: + +* **Declare a plain pointer, not `PREPARE_ERROR`.** That macro *declares* a + fresh context variable set to `NULL`; it cannot adopt one. For the same + reason, never `CATCH` into the variable holding a received context — `CATCH` + assigns over it, and the slot you were handed is gone. +* **In a `void` helper, use `FINISH_NORETURN`.** `FINISH_LOGIC` decides whether + to propagate at run time, so the compiler still *parses* its + `return __err_context` even when the second argument is the literal `false`. + `FINISH(e, false)` in a function returning void therefore draws + `warning: 'return' with a value, in function returning void` from gcc — a + constraint violation, and a build failure under `-Werror`. To propagate + inside the collector's own call stack, give the + helper an `akerr_ErrorContext *` return and use `FINISH(e, true)` as usual — + just never let an error propagate out of the thread body itself, whose + `void *` return nobody reads. `PASS` has the same problem for the same reason: + in a thread body it compiles, hands the pointer back as a `void *`, and leaks + the slot. +* **`FINISH_NORETURN(e)` on a received context still terminates the process.** + That is right — an unhandled error is unhandled wherever it was raised — but + it is now the collector's thread deciding the exit status, not the raiser's. +* **Give the handler blocks their own function.** `ATTEMPT` is a `switch`, and a + `break` inside one written directly in a loop leaves the `switch`, not the + loop. Same hazard as + [do not use CATCH or FAIL_*_BREAK inside a loop](#important-do-not-use-catch-or-fail__break-inside-a-loop). + +**Always** bound the queue. A queued error is a checked-out pool slot, and there +are `AKERR_MAX_ARRAY_ERROR` (128) of them in the entire process. Size *queue +depth + producers with an error in flight* well under that. When the pool runs +dry the library logs and calls `exit(1)` from inside `FAIL` — there is no slot +left to raise the failure *from*, which is exactly why a collector that stops +draining takes the process with it. + +### If you need to keep it as well as report it + +There is no copy or retain call, on purpose: a context is a pool slot, and two +owners of one slot is the thing this whole section exists to prevent. So either +read out what you want to keep — `status` is an `int`, `message` and +`stacktracebuf` are ordinary NUL-terminated strings you can `snprintf` into your +own, much smaller, record — or raise two errors and hand one of them over. + +**Never** copy an `akerr_ErrorContext` by assignment and keep the copy: + +```c +akerr_ErrorContext snapshot = *failed; /* looks fine. is not. */ +``` + +`stacktracebufptr` points into the context's *own* `stacktracebuf`, so after that +assignment `snapshot`'s cursor still points into `failed`'s buffer. +`LOG_ERROR(&snapshot)` reads the array and prints correctly, so it looks healthy +— and then the first `AKERR_STACKTRACE_APPEND(&snapshot, ...)` writes into a +pool slot you no longer own, arbitrarily far from the copy. `arrayid` has the +same shape: it is restored after the wipe, so a copied id makes the destination +impersonate the source's slot forever. And the copy is not a pool address, so +`akerr_valid_error_address()` rejects it, every `CATCH` on it becomes +`AKERR_BADEXC`, and releasing it memsets your own storage while the real slot +stays checked out for the life of the process. + ## Building single threaded The threading backend is chosen when libakerror is configured. `auto` (the @@ -195,7 +335,9 @@ one thread is then undefined. The thread tests (`tests/err_threads_*.c`) assert the properties above directly: exclusive ownership of pool slots, exactly one winner for a contested range, -every registered name readable back. They run in the normal suite. The run that +every registered name readable back, and — in `err_threads_handoff.c` — an error +raised on one thread arriving whole on another and released there. They run in +the normal suite. The run that proves the *absence* of a data race underneath them is ThreadSanitizer: ```sh diff --git a/TODO.md b/TODO.md index 0c61eb0..22c7748 100644 --- a/TODO.md +++ b/TODO.md @@ -112,6 +112,43 @@ can be configured with `-DAKERR_SANITIZE=thread`. The whole run then costs a TSan-instrumented suite per mutant (roughly 6s instead of 0.4s), so it belongs behind a flag rather than in the default target or in CI. +## 9. No way to keep an error context and report it at the same time + +A context can be handed to another thread and released there -- `README.md` now +documents that pattern and `tests/err_threads_handoff.c` proves it -- but it is a +*move*. A thread that wants to both keep its error and report it upward has to +read the fields out into its own record, and it loses the stack trace doing so, +because `stacktracebuf` is the one thing that cannot be usefully summarized. + +Copying the struct is not a workaround. `stacktracebufptr` is self-referential +(`include/akerror.tmpl.h`), so `akerr_ErrorContext c = *src;` leaves the copy's +cursor pointing into the source's buffer -- the copy logs correctly and then +corrupts a slot it does not own the first time anything appends to it. `arrayid` +is restored after the wipe in `akerr_release_error()` (`src/error.c:395-398`), so +a copied id makes the destination impersonate the source's slot for the life of +the process. + +If this is ever worth an API, the shape is: + +```c +akerr_ErrorContext AKERR_NOIGNORE *akerr_copy_error(akerr_ErrorContext *source, + akerr_ErrorContext *destination); +``` + +taking the destination as a parameter rather than allocating it. An allocating +copy could fail on pool exhaustion, and reporting *that* failure needs a pool +slot, so it would have to abort -- adding a third `exit()` site to a library that +deliberately has two. Caller-allocates puts the pool pressure where it can be +managed. The copy must repair four fields: `arrayid` (the destination's own), +`refcount` (set to 1, never inherited), `handled` (false -- a copy is a fresh +obligation, or `FINISH_NORETURN` on the receiving side drops it silently), and +`stacktracebufptr` (re-anchored to the destination's buffer at the *same offset*, +so a later append continues the trace instead of overwriting it). + +Not worth building yet: no consumer needs it. The trigger is a consumer that +needs a worker's stack *trace*, not just its status and message, at the join +point -- `libakstdlib`'s planned `pthread_*` wrappers are the likely first. + ## Unrelated pre-existing issues - The `AKERR_USE_STDLIB=OFF` build does not compile at all: `bool`, `PATH_MAX` diff --git a/UPGRADING.md b/UPGRADING.md index 5e6874d..2dcf50c 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -95,11 +95,20 @@ Safe from any thread, with no coordination on your part: * `akerr_name_for_status(status, NULL)` lookups, concurrently with each other and with registrations of *other* statuses. * `akerr_init()`, from any number of threads at once. +* **Handing a context to another thread, and releasing it there.** The reference + count is the only field the library reads across threads and it is only ever + touched under the pool lock, so `akerr_release_error()` does not care which + thread checked the slot out. Contexts live in process-global storage, not + thread-local, so one outlives the thread that raised it. What is still yours is + the handoff *itself*: it has to carry a happens-before edge, which any mutex, + condvar, `pthread_join` or acquire/release atomic gives you. Still yours to coordinate: -* **One error context is owned by one thread.** The library hands it to the - thread that raised it. Handing it to another thread is your synchronization. +* **Two threads in one context at once.** Ownership moves; it does not fork. + Hand a context over and stop touching it — the content is written with no + lock, so the handoff is what publishes it. See "Handing an error to another + thread" in README.md for the pattern, and for why the queue has to be bounded. * **`akerr_log_method` and `akerr_handler_unhandled_error`** are read on every error and written by nobody but you. Set them during startup, before spawning. * **Renaming a status while another thread looks it up.** @@ -128,9 +137,9 @@ library from that thread. ## Proving it -`tests/err_threads_init.c`, `tests/err_threads_pool.c` and -`tests/err_threads_registry.c` assert the properties directly and run in the -normal suite. The run that proves there is no data race underneath them is +`tests/err_threads_init.c`, `tests/err_threads_pool.c`, +`tests/err_threads_registry.c` and `tests/err_threads_handoff.c` assert the +properties directly and run in the normal suite. The run that proves there is no data race underneath them is ThreadSanitizer: ```sh diff --git a/include/akerror.tmpl.h b/include/akerror.tmpl.h index 7f4e5a5..b8517fd 100644 --- a/include/akerror.tmpl.h +++ b/include/akerror.tmpl.h @@ -159,6 +159,11 @@ typedef struct typedef void (*akerr_ErrorUnhandledErrorHandler)(akerr_ErrorContext *errctx); typedef void (*akerr_ErrorLogFunction)(const char *f, ...); +/* + * The pool. Process-global, not thread-local: a context outlives the thread that + * raised it, which is what lets one be handed to another thread and released + * there. + */ extern akerr_ErrorContext AKERR_ARRAY_ERROR[AKERR_MAX_ARRAY_ERROR]; /* * Set these before starting threads. They are read on every error and written @@ -174,6 +179,17 @@ extern akerr_ErrorLogFunction akerr_log_method; */ extern AKERR_THREAD_LOCAL akerr_ErrorContext *__akerr_last_ignored; +/* + * Drop one reference, returning NULL once the last one is gone so the caller can + * null its own pointer. + * + * This need not be the thread that checked the context out. The reference count + * is the only field the library reads across threads, and it is only ever + * touched under the pool lock, so a context handed to another thread is released + * there. Exactly once, though: releasing a stale pointer takes the + * refcount-zero branch a second time and wipes a slot that by then holds + * somebody else's live error. + */ akerr_ErrorContext AKERR_NOIGNORE *akerr_release_error(akerr_ErrorContext *ptr); /* * Check a context out of the pool. The returned context already carries one diff --git a/tests/err_threads_handoff.c b/tests/err_threads_handoff.c new file mode 100644 index 0000000..91e5b84 --- /dev/null +++ b/tests/err_threads_handoff.c @@ -0,0 +1,254 @@ +#include "akerror.h" +#include "err_capture.h" +#include "err_threads.h" +#include + +/* + * Handing an error context from one thread to another. + * + * A context is not thread state. It lives in AKERR_ARRAY_ERROR, which is + * process-global, and the only field of it the library reads across an + * ownership boundary is the reference count -- which is only ever touched under + * the pool lock. So a context can be raised on one thread, handed to another, + * and handled and released there, and akerr_release_error() does not care which + * thread checked the slot out. That is a property this library promises, and it + * is what makes the worker/collector shape usable at all. + * + * The other half of the promise is what it does *not* cover: two threads inside + * one context at once. Ownership moves, it does not fork. This test asserts the + * supported half; the unsupported half cannot be asserted without deliberately + * racing, which ThreadSanitizer would then correctly fail. + * + * The queue below is a plain mutex and two condition variables rather than the + * __atomic builtins the rest of these tests use, and that is deliberate: the + * mutex *is* the thing under test. Context content is written with no lock at + * all, so the handoff itself is what publishes those writes to the receiver. + * + * The claim is proved from four directions: + * + * 1. The context is still a live pool slot after it crosses, holding exactly + * the one reference it was checked out with. + * 2. Its message and its whole stack trace -- producer frame and all -- arrive + * intact, and in each producer's own order. + * 3. The slot is never recycled underneath the transfer: akerr_slot_owner[] + * still names the producer when the collector picks it up. + * 4. A context outlives the thread that raised it (see main()). + */ + +#define ITERATIONS 500 +#define AKERR_HANDOFF_DEPTH 32 + +/* + * The sizing rule the README gives, made executable. Every queued error is a + * checked-out pool slot, and so is every producer's error in flight. Outrun the + * pool and ENSURE_ERROR_READY exits the process from inside FAIL, with no slot + * left to raise the failure from. + */ +typedef char akerr_assert_handoff_fits_pool[ + (AKERR_HANDOFF_DEPTH + AKERR_TEST_THREADS < AKERR_MAX_ARRAY_ERROR) ? 1 : -1]; + +static struct +{ + pthread_mutex_t lock; + pthread_cond_t not_full; + pthread_cond_t not_empty; + akerr_ErrorContext *slot[AKERR_HANDOFF_DEPTH]; + int head; + int count; +} queue; + +/* Bounded on purpose: an unbounded queue of errors is an unbounded number of + * checked-out pool slots. Blocking the producer is the backpressure. */ +static void queue_push(akerr_ErrorContext *errctx) +{ + pthread_mutex_lock(&queue.lock); + while ( queue.count == AKERR_HANDOFF_DEPTH ) { + pthread_cond_wait(&queue.not_full, &queue.lock); + } + queue.slot[(queue.head + queue.count) % AKERR_HANDOFF_DEPTH] = errctx; + queue.count += 1; + pthread_cond_signal(&queue.not_empty); + pthread_mutex_unlock(&queue.lock); +} + +static akerr_ErrorContext *queue_pop(void) +{ + akerr_ErrorContext *errctx; + + pthread_mutex_lock(&queue.lock); + while ( queue.count == 0 ) { + pthread_cond_wait(&queue.not_empty, &queue.lock); + } + errctx = queue.slot[queue.head]; + queue.head = (queue.head + 1) % AKERR_HANDOFF_DEPTH; + queue.count -= 1; + pthread_cond_signal(&queue.not_full); + pthread_mutex_unlock(&queue.lock); + return errctx; +} + +/* + * Raise an error and give it away. The stack-trace frame is appended before the + * push so the trace records the crossing, and it is the last thing this thread + * does to the context: after queue_push() returns, `e` belongs to the collector + * and reading even e->status here would be the unsupported half of the rule. + */ +static void produce_one(akerr_ThreadArg *arg, int seq) +{ + PREPARE_ERROR(e); + + FAIL(e, AKERR_VALUE, "thread %d seq %d", arg->id, seq); + AKERR_TCHECK(arg, akerr_slot_claim(e->arrayid, arg->id) == 0); + AKERR_STACKTRACE_APPEND(e, "queued by thread %d\n", arg->id); + queue_push(e); +} + +/* + * One received error, handled and released on a thread that never called + * akerr_next_error(). That release is the whole claim. + * + * `seen` is the collector's own per-producer sequence counter. Collector-local + * means no atomics: keeping the producers in order is the queue's job, and + * checking it is this thread's. + */ +static void collect_one(akerr_ThreadArg *arg, akerr_ErrorContext *e, int *seen) +{ + char expected[64]; + int producer = 0; + int seq = 0; + + AKERR_TCHECK(arg, akerr_valid_error_address(e) == 1); + /* It crossed holding exactly the reference it was checked out with. */ + AKERR_TCHECK(arg, e->refcount == 1); + AKERR_TCHECK(arg, sscanf(e->message, "thread %d seq %d", &producer, &seq) == 2); + AKERR_TCHECK(arg, producer >= 2 && producer <= AKERR_TEST_THREADS); + if ( producer >= 2 && producer <= AKERR_TEST_THREADS ) { + AKERR_TCHECK(arg, seq == seen[producer]); + seen[producer] += 1; + } + /* The slot still belongs to the producer, so nothing recycled it while it + * was in flight. */ + AKERR_TCHECK(arg, akerr_slot_holder(e->arrayid) == producer); + + snprintf(expected, sizeof(expected), "thread %d seq %d", producer, seq); + /* Nothing to attempt -- the error is already in hand. The blocks are here + * because this is the assembly the macros require, and because a real + * collector reads exactly like this. */ + ATTEMPT { + } CLEANUP { + } PROCESS(e) { + /* case 0: a handed-off error that arrives with no status means somebody + * wrote over the context after the producer let it go. */ + int error_was_lost = 1; + AKERR_TCHECK(arg, error_was_lost == 0); + } HANDLE(e, AKERR_VALUE) { + /* HANDLE rewinds the cursor, but the bytes are still there: the whole + * trace crossed with the context, producer frame and handoff frame. */ + AKERR_TCHECK(arg, strstr(e->stacktracebuf, expected) != NULL); + AKERR_TCHECK(arg, strstr(e->stacktracebuf, "queued by thread") != NULL); + /* Give the slot up before FINISH releases the context: the other order + * hands it back to the pool while this thread still claims it. */ + akerr_slot_drop(e->arrayid); + } FINISH_NORETURN(e); + /* FINISH_NORETURN, not FINISH(e, false): FINISH_LOGIC decides whether to + * propagate at run time, so the compiler still parses its + * `return __err_context` and diagnoses it in a function returning void, + * whatever __pass_up says. An error this collector did not handle takes the + * process down from here, which is right -- but note it is now the + * collector's thread deciding the exit status. */ +} + +/* + * Drain exactly what the producers will send. A fixed count rather than a + * sentinel: a miscounted handoff should fail the test, not hang it. + */ +static void collect_all(akerr_ThreadArg *arg) +{ + int seen[AKERR_TEST_THREADS + 1] = { 0 }; + int total = (AKERR_TEST_THREADS - 1) * ITERATIONS; + + for ( int i = 0; i < total; i++ ) { + collect_one(arg, queue_pop(), seen); + } +} + +static void *handoff_body(void *raw) +{ + akerr_ThreadArg *arg = raw; + + pthread_barrier_wait(arg->barrier); + + if ( arg->id == 1 ) { + collect_all(arg); + } else { + for ( int i = 0; i < ITERATIONS; i++ ) { + produce_one(arg, i); + } + } + return NULL; +} + +/* + * Written by the raising thread, read by main() after pthread_join(). The join + * is the happens-before edge, which is the same thing the queue's mutex does + * above -- a plain global needs no atomics once something orders it. + */ +static akerr_ErrorContext *parked; + +static void *raise_and_exit(void *unused) +{ + PREPARE_ERROR(e); + + (void)unused; + FAIL(e, AKERR_IO, "raised on a thread that exited"); + parked = e; + return NULL; +} + +int main(void) +{ + pthread_t raiser; + int failures = 0; + + akerr_log_method = &akerr_thread_logger; + akerr_init(); + AKERR_CHECK(akerr_slots_in_use() == 0); + AKERR_CHECK(pthread_mutex_init(&queue.lock, NULL) == 0); + AKERR_CHECK(pthread_cond_init(&queue.not_full, NULL) == 0); + AKERR_CHECK(pthread_cond_init(&queue.not_empty, NULL) == 0); + + failures = akerr_run_threads(&handoff_body); + AKERR_CHECK(failures == 0); + /* Every handed-off context was released by the thread that received it. */ + AKERR_CHECK(akerr_slots_in_use() == 0); + + /* + * A context outlives the thread that raised it: the pool is process-global, + * not thread-local storage. By the time these checks run, the thread that + * called FAIL() no longer exists. + */ + AKERR_CHECK(pthread_create(&raiser, NULL, &raise_and_exit, NULL) == 0); + AKERR_CHECK(pthread_join(raiser, NULL) == 0); + AKERR_CHECK(parked != NULL); + AKERR_CHECK(akerr_valid_error_address(parked) == 1); + AKERR_CHECK(parked->status == AKERR_IO); + AKERR_CHECK(parked->refcount == 1); + AKERR_CHECK(strstr(parked->stacktracebuf, "raised on a thread that exited") != NULL); + RELEASE_ERROR(parked); + AKERR_CHECK(parked == NULL); + + AKERR_CHECK(akerr_slots_in_use() == 0); + for ( int i = 0; i < AKERR_MAX_ARRAY_ERROR; i++ ) { + AKERR_CHECK(akerr_slot_holder(i) == 0); + } + /* Nothing here reports through the log method: a handoff is not an error. */ + AKERR_CHECK(akerr_thread_logs() == 0); + + pthread_cond_destroy(&queue.not_empty); + pthread_cond_destroy(&queue.not_full); + pthread_mutex_destroy(&queue.lock); + + fprintf(stderr, "err_threads_handoff ok (%d producers x %d errors)\n", + AKERR_TEST_THREADS - 1, ITERATIONS); + return 0; +}