Skip to content

fix data race in SqliteLogger::callback - #1237

Open
aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:sqlite-logger-callback-race
Open

aysha-afrah26 wants to merge 1 commit into
BehaviorTree:masterfrom
aysha-afrah26:sqlite-logger-callback-race

Conversation

@aysha-afrah26

Copy link
Copy Markdown
Contributor

Status-change callbacks are not serialized: a ThreadedAction that completes calls setStatus from its worker thread, so two actions finishing at the same time (or one finishing while the main thread ticks) invoke SqliteLogger::callback concurrently. The callback updated monotonic_timestamp_ and the starting_time_ map before taking queue_mutex_, which is a data race on the timestamp and can corrupt the unordered_map on a concurrent insert. When two racing callbacks compute the same timestamp, the later INSERT also violates the PRIMARY KEY of the Transitions table and the uncaught exception in the writer thread terminates the process. Groot2Publisher and FileLogger2 already lock inside their callback for the same reason. The fix computes the timestamp and duration under the queue_mutex_ that the function already takes to push the transition, so there is no new member and no API or ABI change; single-threaded behavior is unchanged. Added the regression test SqliteLogger_ConcurrentTransitions, which ThreadSanitizer flags at the monotonic_timestamp_ update before the fix and reports clean after (the sanitizers CI job runs the suite under TSan). Full ctest on macOS is 546/547 in Release and 549/551 under TSan, with the failures (SqliteLogger_ExecSqlStatement, Parallel.FailingParallel, MultipleLoggers) verified to fail identically on unpatched master. pre-commit (clang-format, codespell) passes on the touched files.

This branch has not been deployed

No deployments
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