Repository navigation
test: add multithreaded tests, fix two thread-safety bugs - #42
Conversation
751f205 to
0dc7bfd
Compare
bce588f to
282b727
Compare
0dc7bfd to
0b6d3ec
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The concurrency rework of the shared registry and framework-wide logger redesign depend on sanitizer/TSan verification that has not yet passed in CI, leaving correctness unconfirmed.
No open issues 🎉
What changed in this PR
This PR adds a dedicated multithreading test suite that exercises the algorithms core framework as it would be used by concurrent frameworks (shared process(), concurrent construction/configuration, concurrent property reads, concurrent logging, concurrent first-access of services, and shared/parallel random generators). Running these under the thread sanitizer surfaced two real races, which are fixed in the same PR: non-thread-safe service registration and interleaved logger stream messages.
Changes:
- Guard the
ServiceSvcregistry with amutable std::recursive_mutex(recursive because initializers can register other services) across all registry reads/writes. - Redesign
LoggerMixin/LoggerStreamso eachcritical()/…/trace()call returns a fresh stream by value with a lazily-created buffer (suppressed messages stay cheap) that flushes unflushed output on destruction;LoggerMixinis explicitly non-copyable. - Add
core/tests/multithreading.cpp, a concurrent block inalgorithm_svc.cpp, a new unflushed-stream test inlogger.cpp, and linkThreads::Threads.
| File | Description |
|---|---|
core/include/algorithms/service.h |
Adds recursive-mutex locking to registry mutators/accessors; documents services() as unprotected. |
core/include/algorithms/logger.h |
Per-call LoggerStream returned by value with lazy buffer and destructor flush; LoggerMixin made explicitly non-copyable; level() simplified. |
core/tests/multithreading.cpp |
New suite covering concurrent algorithms, logging, properties, services, and RNG. |
core/tests/logger.cpp |
New test for reporting unflushed stream output at statement end. |
core/tests/algorithm_svc.cpp |
Adds concurrent create/list/add lifecycle checks. |
core/tests/CMakeLists.txt |
Finds and links Threads::Threads for all test executables. |
Review effort: Balanced
Services register themselves in the ServiceSvc on first access. When different services were first accessed from several threads at the same time, the registry (keys, services and initializers) was modified concurrently, which corrupted it (data races, use after free). Guard the registry with a recursive mutex; recursive, as initializers can access, and thereby register, other services. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LoggerMixin returned one shared stream per log level, so messages that several threads composed concurrently on the same logger (e.g. in an algorithm shared between threads) got mixed up: the stream only locked each insertion, not a whole message. critical(), error(), ..., trace() now return a new stream per call. Its output buffer is only created for messages above threshold, so suppressed messages stay cheap, and output that is not flushed with endmsg is reported at the end of the statement instead of being prepended to the next message. LoggerMixin stays non-copyable, as it was through its stream members. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test concurrent use as in multithreaded frameworks: processing with one algorithm instance from several threads, constructing and configuring algorithms, reading properties, logging, first access of services, sharing generators and their engine, and creating algorithms from the AlgorithmSvc. The thread sanitizer job runs these tests to detect races that do not change results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0b6d3ec to
41f156b
Compare
Stacked on #41.
Summary
New
core/tests/multithreading.cpptests concurrent use as in multithreaded frameworks:process()on one shared algorithm instance from 8 threads;Generatorshared between threads, and per-thread generators sharing one engine. Full-range draws must all be distinct.The
AlgorithmSvclifecycle test also creates, lists and adds algorithms concurrently. Thetsanjob from #41 runs all of this to catch races that do not change results.The tests found two thread-safety bugs, fixed here in separate commits:
ServiceSvc::registerServicemodified the registry concurrently, which corrupted it (TSan: data races, heap-use-after-free). The registry is now guarded by a recursive mutex. It is recursive because initializers can access, and so register, other services.LoggerMixinreturned one shared stream per log level and locked each<<, not whole messages. Threads logging through one logger produced fragments like"6stream 0".critical()...trace()now return a new stream per call:endmsgis reported at the end of the statement, instead of being prepended to the next message.LoggerMixinstays non-copyable.x() << ... << endmsg, which is unaffected.Test plan
eic_xl:nightly: 95/95 tests pass with TSan, ASan+UBSan, clang++ Debug, g++ Debug and g++ Release🤖 Generated with Claude Code