Andrew Kesterson
caef63826f
The thread-safety section filed two different things under "does not cover, and cannot": sharing a context between threads, and passing one to another thread. Only the first is unsupported. Transfer already works by construction -- the reference count is the only field the library reads across an ownership boundary, and it is only ever touched under the pool lock, so akerr_release_error() does not care which thread checked the slot out. The pool is process-global, not thread-local, so a context outlives the thread that raised it. Calling that unsupported told readers the worker/collector shape was off the table, which either cost them the pattern or cost them the stack trace when they rolled their own struct instead. Split the bullet: transfer joins the covered list and gets its own section with the rule, the worked pattern, and the four receiving-side hazards (PREPARE_ERROR cannot adopt, CATCH assigns over the pointer, FINISH in a void helper still parses its return, and an unhandled error now terminates from the collector's thread). Sharing keeps the "cannot" bullet, narrowed to what it actually is. err_threads_handoff.c proves it: the existing thread tests all keep every context on the thread that raised it, so the transfer path was exercised nowhere. Seven producers hand errors to one collector through a bounded mutex/condvar queue -- the mutex is the thing under test, since it is what publishes the unlocked content writes -- and the collector asserts the context is still a live slot at refcount 1, that message and trace arrive whole and in each producer's order, that the slot was never recycled in flight, and that a thread which never called akerr_next_error() can release it. A second phase reads a context whose raising thread has already exited. Also document why copying a context by assignment is silently wrong: stacktracebufptr is self-referential, so the copy's cursor points into the source's buffer and the first append corrupts a slot the copier no longer owns. TODO.md records the akerr_copy_error() shape that would fix it and the trigger for building it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
160 lines
8.4 KiB
Markdown
160 lines
8.4 KiB
Markdown
# TODO
|
|
|
|
Working notes for `libakerror`. Outstanding items only.
|
|
|
|
## 1. Only ThreadSanitizer is wired into CI, not ASan/UBSan
|
|
|
|
`AKERR_SANITIZE` builds the library and the tests with any sanitizer list, and
|
|
CI runs `-DAKERR_SANITIZE=thread` through `scripts/thread_test.sh`. Nothing runs
|
|
`address,undefined` yet, and that is the one that covers the original
|
|
motivation: mutation testing caught an out-of-bounds probe in the status-name
|
|
hash table that the suite could not, because the failure mode was a write into
|
|
adjacent BSS, which does not crash. Sharpening one test closed that instance;
|
|
ASan would catch the whole class regardless of how sharp the assertions are.
|
|
|
|
The machinery is in place — this is one more job in
|
|
`.gitea/workflows/ci.yaml` running
|
|
`cmake -S . -B build/asan -DAKERR_SANITIZE=address,undefined`. Left separate
|
|
because ASan and TSan cannot be combined in one build.
|
|
|
|
## 2. `HANDLE`-level status aliasing is still undetectable
|
|
|
|
Two components can compile the same integer into a `case` label without ever
|
|
reserving a range or registering a name, and nothing sees it. Ownership
|
|
enforcement covers *naming*, which is the part the library mediates; the `case`
|
|
label never reaches it.
|
|
|
|
Closing this needs the `if`/`else if` handler ladder — rewriting
|
|
`PROCESS`/`HANDLE`/`HANDLE_GROUP`/`HANDLE_DEFAULT`/`FINISH` so status matching
|
|
is not restricted to integer constant expressions. That would also allow
|
|
matching on ranges or predicates, and would let a handler resolve a code through
|
|
its owner. It touches the most load-bearing code in the library and every
|
|
consumer's error handling at once, so it wants its own change.
|
|
|
|
Note it would *not* by itself fix the "don't use `CATCH` or `FAIL_*_BREAK`
|
|
inside a loop" hazard: that comes from exiting via `break`, not from `switch`.
|
|
|
|
## 3. No registry introspection
|
|
|
|
There is no way to ask who owns a status, or to enumerate reservations. The
|
|
"coordinate ranges at the dependency-stack level" advice in UPGRADING.md
|
|
therefore has no tooling behind it.
|
|
|
|
A read-only accessor plus a dump through `akerr_log_method` would let a startup
|
|
self-check or a CI job print the whole map for a linked stack. Cheap, additive,
|
|
and the natural next step for multi-component adoption.
|
|
|
|
## 4. No way to release a reservation
|
|
|
|
A plugin host that `dlopen`s many distinct plugins over a process lifetime
|
|
accumulates ranges until the table fills. Reloading the *same* plugin is fine —
|
|
an identical repeat by the same owner is idempotent.
|
|
|
|
## 5. Renaming a status is not safe against a concurrent lookup
|
|
|
|
`akerr_name_for_status(status, NULL)` returns a pointer into the registry rather
|
|
than a copy, which is what makes it usable from inside `FAIL` — it needs no
|
|
buffer and no error context of its own. Registering a *second* name for a status
|
|
that already has one (`tests/err_name_ownership.c` covers that it is allowed)
|
|
overwrites that buffer in place, so a thread reading the name at that moment can
|
|
see a torn string. Every other registry operation is serialized; this one cannot
|
|
be, because the reader is outside the lock by the time it reads the characters.
|
|
|
|
Documented in README.md and UPGRADING.md as "register names during
|
|
initialization". Closing it properly means making a registered name immutable —
|
|
either refusing a rename outright (a behavior change, and
|
|
`tests/err_name_ownership.c` asserts the current contract), or copying names
|
|
into a bump-allocated arena and publishing the pointer with a release store, so
|
|
a rename allocates new storage instead of rewriting live storage. The arena is
|
|
the better answer; it costs a second capacity limit and its exhaustion path.
|
|
|
|
## 6. Deprecate the two-argument name-registration path
|
|
|
|
`akerr_name_for_status(status, name)` cannot identify its caller, so it can only
|
|
check that *some* reservation covers the status, not that the caller owns it. It
|
|
exists for migration. Once consumers have moved to
|
|
`akerr_register_status_name()`, make the set path a no-op or remove it and leave
|
|
`akerr_name_for_status()` as pure lookup.
|
|
|
|
## 7. `akerr_init()`'s own reservation failure is untested
|
|
|
|
`tests/err_library_status_fatal.c` covers the terminal path in
|
|
`__akerr_name_library_status()` by naming a status the library does not own. The
|
|
band reservation in `akerr_init()` has no such handle: it can only fail in a
|
|
build whose tables are too small for the library's own entries, and both sizes
|
|
are `PRIVATE` to the library target, so a test executable cannot set them.
|
|
|
|
Closing it means a second library target built with tiny tables plus a
|
|
`WILL_FAIL` test linked against it. Nothing in the CMake does that yet: the
|
|
sanitizer and coverage options vary the *flags* of the one library target, not
|
|
its compile definitions.
|
|
|
|
Related: branch coverage on `src/error.c` now sits just above its 50% gate.
|
|
Every `FAIL_*` site carries about six branch outcomes of error-construction
|
|
machinery (`ENSURE_ERROR_READY`, `AKERR_STACKTRACE_APPEND`) that only run when
|
|
that specific failure fires, and every `PASS` site around a call that cannot
|
|
fail carries about twenty-five. Validating more inputs therefore lowers the
|
|
ratio by construction. Before adding defensive checks, expect to add a test that
|
|
drives them, as `tests/err_copy_string.c` does.
|
|
|
|
## 8. Mutation testing judges concurrency mutants without a sanitizer
|
|
|
|
`scripts/mutation_test.py` configures each mutant build with the default CMake
|
|
options, so a mutant that only breaks under concurrency is judged by a suite
|
|
running without ThreadSanitizer. Deleting the pool's `akerr_mutex_lock()` call
|
|
survives the run even though it is a real race: rebuilt and run directly, that
|
|
mutant fails `tests/err_threads_pool.c` in 4 of 10 runs, and fails under
|
|
`scripts/thread_test.sh` in 5 of 5. So 81.2% is a floor for that category, not a
|
|
verdict.
|
|
|
|
Closing it means a `--cmake-arg` passthrough on the harness so the mutant build
|
|
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`
|
|
and `NULL` are used unconditionally but only included under the stdlib branch.
|
|
The README's dependency list states what a replacement must provide, but the
|
|
header still needs its includes untangled for that configuration to work.
|
|
- `CMakeLists.txt` sets `main_lib_dest` from `MY_LIBRARY_VERSION`, which is never
|
|
defined and never read. Dead line.
|