Repository navigation
bridge/engine: External FB pool lock; faults never lost or blocked across dispatch shutdown; test suite race-free - #184
Conversation
…utdown; bounded shutdown (failing)
…t or blocked across dispatch shutdown; shutdown bounded; race-free tests and example accessor
ThreadSanitizer — full suite inventory (
|
| # | Kind | Location | Writer frame | Other frame | Count |
|---|---|---|---|---|---|
| 1 | data race | g_dispatch_count |
count_run_fn, tests/unit/test_dispatch_shutdown.c:90 |
test_reboot_after_shutdown, :276 |
1 |
| 2 | data race | g_sim_running |
test_temp_readings_received, tests/integration/test_thermostat_full.c:199 |
sim_run, :135 |
1 |
| 3 | data race | g_reading_count |
ctrl__reading_count, examples/thermostat/fb_temp_controller.c:212 |
ctrl_run, :152 |
1 |
| 4 | thread leak | embediq_osal_task_create (simulation task never joined) |
osal/posix/embediq_osal_posix.c:156 | — | 1 |
After — Linux CI tsan job on this branch (run 41395024132): TSan report files: 0 reports: 0 distinct findings: 0, 33/33 tests pass. Local macOS TSan build, full suite: 0 findings. tools/ci/tsan.supp has no rules.
The new concurrent pool test, on the unfixed pool code under TSan: 7 distinct races (8 reports) in embediq_ext_fb.c (s_pool_initialised, s_pool[].in_use and the slot fields). After the fix: 0.
New tests, before → after
| Test | Before the fix | After the fix |
|---|---|---|
test_ext_fb_init_concurrent_unique_slots |
8 handles claimed, duplicate virtual endpoint ids | 8 distinct slots and ids |
test_fault_queued_before_shutdown_is_applied |
a fault reported while shutdown was in progress was never applied (during_shutdown_calls=0) |
applied once; the fault queued before shutdown also applied once |
test_report_fault_never_waits_for_queue_capacity |
post never refused; the reporter waited for queue space and the fault ran on the dispatch thread | post refused at once; fault applied on the reporter's thread; returned 1.2 ms after the running handler finished |
test_dispatch_shutdown_returns_under_reporter_storm |
shutdown never returned (killed by the 2 s alarm) | shutdown 0 ms; 13,366 reports = 13,366 fault_fn calls; 20 NORMAL = 9 delivered + 11 in one EMBEDIQ_QUEUE_DROP_AT_SHUTDOWN event |
Adversarial runs after the fix (normal and TSan builds, 0 TSan reports)
- 50 boot/shutdown cycles, 8 reporters and a continuous NORMAL publisher: 915,800 reports in total; faults applied == reported in every cycle. Max shutdown latency 1 ms (3 ms under TSan).
- Nested fault during the shutdown drain: a queued fault whose
fault_fnreports a second fault on the same FB. Both are applied on the FB's dispatch thread before it exits. fault_fnpublishing HIGH into another FB's full queue during shutdown: thatfault_fnblocks (only its own FB's code) until the other FB's running handler returns. Shutdown then completes 2 ms later. The other FB's drain discards and reports 17 messages: its 16 queued plus the late publish.
For reviewers
- Pool initialisation uses an atomic one-time init state, not the OSAL critical section.
embediq_osal.hforbids OSAL calls and loops over data inside a critical section, and initialisation creates the pool mutex and clears the pool. EMBEDIQ_SHUTDOWN_INFLIGHT_WAIT_MS(100 ms) is inembediq_config.hwith the other bounds. New engine fault codeEMBEDIQ_FAULT_SHUTDOWN_INFLIGHT= 0x82; newQUEUE_DROPflagEMBEDIQ_QUEUE_DROP_AT_SHUTDOWN= 3.- The thermostat demo never calls
embediq_engine_dispatch_shutdown(), so its 30 s capture contains noQUEUE_DROP_AT_SHUTDOWNrecord. The capture decodes cleanly: 342 events, sequence contiguous, 0 inversions. - Other checks: normal ctest 32/32. ASan clean on test_fb_engine, test_fb_bridge, test_dispatch_shutdown and test_thermostat_full. validator, boundary_checker, check_fb_calls, check_osal_obs
--strictand check_hal_obs--strictall clean.embediq_fb.h,embediq_obs.handembediq_ext_fb.heach compile standalone.
|
Post-merge follow-up review at [P1] Do not permit an in-flight reporter to enqueue after the final shutdown drain The 100 ms in-flight wait breaks on timeout, drains once more, clears the flags, and returns. A reporter that already passed the running/shutdown checks can remain descheduled until after that final drain, then resume posting successfully to a queue with no dispatcher. A scheduling shim pauses The shutdown timeout event diagnoses the stall but does not preserve fault delivery. Sequential consistency and clearing running before shutdown do not stop a reporter already past those loads. Please coordinate post acceptance with the shutdown generation/closing gate so every accepted fault is owned by a drain or safely redirected. Keep bounded shutdown without allowing a late orphaned enqueue. Add a deterministic test delaying a reporter beyond [P1] Full-HIGH fallback can deadlock two subscribed FBs on their execution locks The refused-post fallback invokes Shutdown then cannot join these dispatchers. The documented AB/BA restriction mentions two FBs without dispatch threads; these both have dispatch threads. This is a residual liveness hole in the full-queue fallback, even though the older blocking-send path could deadlock on capacity too. Please avoid synchronously acquiring a second FB's execution lock from FB-owned code; use owner-directed deferred delivery with an explicit overflow policy. Test this full-queue, two-dispatcher case. The pool claim/release repair and existing fault-shutdown tests pass locally; engine/bridge/NVM/watchdog binaries pass targeted TSan runs with zero reports. Neither of these deterministic scenarios is covered by those tests. |
The bugs
embediq_ext_fb_init()claimed pool slots with an unlocked scan-and-set, so two External FB clients starting atonce could share a slot and a virtual endpoint id. Dispatch shutdown lost and blocked: a fault posted to an FB's
queue shortly before
embediq_engine_dispatch_shutdown()was never applied; a reporter that lost the race toshutdown could block forever on a full HIGH queue; and shutdown itself did not return while reporters kept a queue
non-empty. The test suite and the thermostat example's test accessor had data races of their own.
What breaks, and when
Cross-routed bus traffic between two sidecar processes. A fault raised during an orderly shutdown vanishes; a
platform thread hangs in
report_fault; shutdown hangs under load. A ThreadSanitizer gate that is red for reasonsunrelated to the framework.
What changed and why it fixes it
Pool initialisation is one-time and race-free (an atomic init state), and slot claim/release are serialised by a
mutex. Fault posts never wait for queue capacity: a fault that cannot be queued is applied on the caller's thread
under the FB's execution lock. Shutdown has one linearization point — the shutdown flag — and an in-flight post
counter: every post that completed before the flag is seen by a drain that applies remaining framework faults and
reports discarded messages as one QUEUE_DROP event per FB; the flags are cleared in the order that leaves no window
for an orphaned post. The dispatch loop tests the flag before every message, so shutdown is bounded by queue
capacity, not by reporter behaviour. Test flags are atomics, the simulation task is joined, the example accessor
reads an atomic on host builds — no lock in any FB handler.
Evidence
Four new tests (concurrent pool claim; queued fault applied across shutdown, and a fault reported while shutdown is
in progress applied; a fault reported against a full HIGH queue is refused at once and applied on the caller's
thread, returning < 50 ms after the FB's running handler finishes; shutdown returns in < 500 ms under four reporters
with faults applied == reported). Before/after ThreadSanitizer inventory for the full suite: table in the first PR
comment, target 0 findings.
Limits stated
Messages other than framework faults that are still queued at shutdown are discarded and counted, not delivered.
Shutdown waits at most EMBEDIQ_SHUTDOWN_INFLIGHT_WAIT_MS for in-flight fault posts (microseconds in practice) and
reports if the bound is reached.
🤖 Generated with Claude Code