yujinrobot / kobuki

Software for iClebo Kobuki
kobuki.yujinrobot.com
222 stars 176 forks source link

Lack of Thread Synchronization #390

Open git-afsantos opened 6 years ago

git-afsantos commented 6 years ago

Kobuki's nodelets seem to lack proper thread synchronization mechanisms (e.g. mutex), to protect against the reading of inconsistent values (mid-write) and stale data (writes made by one thread are not visible to another). This is true for kobuki_safety_controller, kobuki_random_walker and kobuki_node (this one to a lesser extent).

Are these mechanisms provided somewhere else (e.g. super-classes), and I just missed them? Otherwise, these nodelets are relying on chance to behave correctly.

Example (with kobuki_safety_controller)

After the robot bumps into an obstacle, the safety controller receives a bumper event and SafetyController::bumperEventCB is called on the "main" thread of the nodelet. This callback is supposed to update, say, bumper_center_pressed_ = true;.

The update thread will, eventually, call spin, where it attempts to read bumper_center_pressed_. Since there are no synchronization mechanisms of any sort, there is no guarantee that this thread will ever see true on this variable (if it relies on the processor's cache). This will result in the safety action not being triggered, leaving the robot to keep pushing the obstacle.

clalancette commented 4 years ago

Absolutely, this is a problem I've noticed as well. I'll assign this to myself to work on (though my work will be delayed a bit).

stonier commented 4 years ago

Aye - I concur. I'll also keep an eye out when moving on the ROS2 upgrade.