Skip to content

FileLogger2: drain the transition queue in the destructor after joining the writer thread - #1198

Open
simobenziane wants to merge 1 commit into
BehaviorTree:masterfrom
simobenziane:fix/filelogger2-drain-after-join
Open

FileLogger2: drain the transition queue in the destructor after joining the writer thread#1198
simobenziane wants to merge 1 commit into
BehaviorTree:masterfrom
simobenziane:fix/filelogger2-drain-after-join

Conversation

@simobenziane

Copy link
Copy Markdown

FileLogger2::writerLoop() checks loop between batches, and ~FileLogger2() sets loop = false and joins without draining transitions_queue. A transition pushed after the writer's last swap — typically the root's RUNNING→IDLE from a haltTree() issued right before the logger is destroyed — is lost, so the .btlog ends mid-flight. Reproduced with a serve loop that halts the tree on a broken pipe and then resets its logger; under load the last IDLE is missing in a fraction of runs (12 of 300 with the destructor instrumented, every failure with a non-empty queue at the join).

This PR extracts the batch write into writeBatch(), adds drainQueue() (swap under the mutex, then write), and calls it in the destructor after the join, before the file is closed. No behaviour change for callers. A looping gtest (LoggerTest.FileLogger2_DrainsQueueAfterHaltThenImmediateDestroy, 300 iterations) ticks a small tree to RUNNING, halts it, destroys the logger at once and asserts the root's last transition in the file is IDLE — it fails on the unpatched code under CPU load and passes with the patch.

🤖 Generated with Claude Code

…oining the writer thread

FileLogger2::writerLoop() checks `loop` between batches, and ~FileLogger2() sets
`loop = false` and joins without draining transitions_queue. A transition pushed
after the writer's last swap -- typically the root's RUNNING->IDLE from a
haltTree() issued right before the logger is destroyed -- is lost, so the
.btlog ends mid-flight. Reproduced with a serve loop that halts the tree on a
broken pipe and then resets its logger; under load the last IDLE is missing in
a fraction of runs.

This extracts the batch write into writeBatch(), adds drainQueue() (swap under
the mutex, then write), and calls it in the destructor after the join, before
the file is closed. No behaviour change for callers.

A new looping test, FileLogger2_DrainsQueueAfterHaltThenImmediateDestroy,
ticks a tree to RUNNING, halts it, and destroys the logger immediately, then
reads the .btlog back and asserts the root's last transition is IDLE, 300
times per run to exercise the scheduling gap. On the unpatched destructor it
fails intermittently under load (3/10 and 10/10 process invocations failed
across two load runs); with the fix, 3000/3000 iterations passed under the
same load.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@simobenziane

Copy link
Copy Markdown
Author

CI note: the one red check, coverage, fails on Decorator.DestroyTreeWhileTimerIsPending (an abort in timer_queue.h:209, TimerQueue::run()), which this PR does not touch; the new LoggerTest.FileLogger2_DrainsQueueAfterHaltThenImmediateDestroy passed in that same job and every other check is green. Happy to rebase if that test is fixed on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant