From 18bc6cf8724d803d4c30650369ca0db3fa62df7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beat=20K=C3=BCng?= Date: Mon, 9 Mar 2020 12:06:15 +0100 Subject: [PATCH] ScheduledWorkItem::ScheduleClear: remove item from runnable queue The existing behavior is unexpected: if the work item is already on the runnable queue, it will still be triggered after a call to ScheduleClear(). This can lead to race conditions. --- .../px4_platform_common/px4_work_queue/WorkItem.hpp | 4 ++++ platforms/common/px4_work_queue/ScheduledWorkItem.cpp | 2 ++ platforms/common/px4_work_queue/WorkItem.cpp | 8 ++++++++ platforms/common/px4_work_queue/WorkQueue.cpp | 1 + 4 files changed, 15 insertions(+) diff --git a/platforms/common/include/px4_platform_common/px4_work_queue/WorkItem.hpp b/platforms/common/include/px4_platform_common/px4_work_queue/WorkItem.hpp index 0430dd1689..498ceea92c 100644 --- a/platforms/common/include/px4_platform_common/px4_work_queue/WorkItem.hpp +++ b/platforms/common/include/px4_platform_common/px4_work_queue/WorkItem.hpp @@ -81,6 +81,10 @@ protected: virtual ~WorkItem(); + /** + * Remove work item from the runnable queue, if it's there + */ + void ScheduleClear(); protected: void RunPreamble() { _run_count++; } diff --git a/platforms/common/px4_work_queue/ScheduledWorkItem.cpp b/platforms/common/px4_work_queue/ScheduledWorkItem.cpp index 99c4f45e0e..43152ce5eb 100644 --- a/platforms/common/px4_work_queue/ScheduledWorkItem.cpp +++ b/platforms/common/px4_work_queue/ScheduledWorkItem.cpp @@ -66,7 +66,9 @@ ScheduledWorkItem::ScheduleOnInterval(uint32_t interval_us, uint32_t delay_us) void ScheduledWorkItem::ScheduleClear() { + // first clear any scheduled hrt call, then remove the item from the runnable queue hrt_cancel(&_call); + WorkItem::ScheduleClear(); } void diff --git a/platforms/common/px4_work_queue/WorkItem.cpp b/platforms/common/px4_work_queue/WorkItem.cpp index 3d8a5e4406..bccb3dd8e7 100644 --- a/platforms/common/px4_work_queue/WorkItem.cpp +++ b/platforms/common/px4_work_queue/WorkItem.cpp @@ -89,6 +89,14 @@ WorkItem::Deinit() } } +void +WorkItem::ScheduleClear() +{ + if (_wq != nullptr) { + _wq->Remove(this); + } +} + float WorkItem::elapsed_time() const { diff --git a/platforms/common/px4_work_queue/WorkQueue.cpp b/platforms/common/px4_work_queue/WorkQueue.cpp index 5f775f2091..63e5ff1531 100644 --- a/platforms/common/px4_work_queue/WorkQueue.cpp +++ b/platforms/common/px4_work_queue/WorkQueue.cpp @@ -160,6 +160,7 @@ WorkQueue::Run() work_unlock(); // unlock work queue to run (item may requeue itself) work->RunPreamble(); work->Run(); + // Note: after Run() we cannot access work anymore, as it might have been deleted work_lock(); // re-lock }