fix: prevent stale until_promise_resolved watchers from halting later spins - #716
fayezzouari wants to merge 4 commits into
Conversation
… spins When a spin with `until_promise_resolved` ended for another reason (e.g. a timeout), the task watching the promise stayed alive and kept a handle to the executor-wide `halt_spinning` flag. Once the promise resolved, it halted whichever spin was running at that time, or the next one to start. Give each spin its own halt flag. `ExecutorCommands::halt_spinning()` sets the flag of the current spin, and a leftover watcher can only set the flag of the spin that created it. Assisted-by: Claude:claude-opus-5-5
| async_worker_commands: Arc<WorkerCommands>, | ||
| halt_spinning: Arc<AtomicBool>, | ||
| /// Halt flag of the most recent spin. | ||
| current_halt: Mutex<Arc<AtomicBool>>, |
There was a problem hiding this comment.
We shouldn't need to use a mutex around an AtomicBool. You should be able to remove the Mutex<_> and then we won't need to lock anything in order to store or load.
There was a problem hiding this comment.
Oh sorry, I missed the fact that we change which Arc is being held by the field. Please ignore the previous comment.
Revert the ExecutorCommands changes so the shared halt_spinning flag no longer needs a Mutex. Instead, each BasicExecutorRuntime::spin creates a token, and the until_promise_resolved watcher holds a Weak reference to it. When the promise resolves, the watcher only halts spinning if its own spin is still alive. Async tasks are only polled inside spin(), so this check cannot race with a spin ending. Assisted-by: Claude:claude-opus-5-5
|
|
||
| fn make_spin_conditions(&self, options: SpinOptions) -> SpinConditions { | ||
| self.commands.halt_spinning.store(false, Ordering::Release); | ||
| let halt_spinning = Arc::new(AtomicBool::new(false)); |
There was a problem hiding this comment.
I know a bool isn't much of a memory footprint to be concerned about, but I worry about having a mandatory heap allocation each time that spin conditions are created. That takes us one step away from potentially supporting real time execution (not that we support it yet...).
I appreciate the elegance of this solution, and we definitely want to fix this problem, but I'd like to put a little thought into whether we can fix this effectively without an allocation. Perhaps we can introduce a second atomic variable 🤔
There was a problem hiding this comment.
Yes, it is honestly fair to think of it that way. I switched to another method, now the executor keeps a spin counter. Each time spin() starts, the counter goes up by one: spin 1, spin 2, spin 3, and so on. When a watcher is created, it remembers the current number, for example "I belong to spin 1". When the promise resolves, the watcher checks the counter first:
- If it still says 1, its spin is still running, so it stops the spin as intended.
- If it says 2 or more, its spin is already over, so it does nothing.
This reverts commit e151f8f.
… spin Revert the per-spin halt flag in ExecutorCommands, which allocated on every spin. BasicExecutorRuntime now owns a spin counter that is allocated once. The until_promise_resolved watcher records which spin created it and only halts spinning if that spin is still running. Assisted-by: Claude:claude-opus-5-5
2 participants
Fixes #715
If a spin with
until_promise_resolvedends for another reason (e.g. a timeout), the watcher task keeps the executor-widehalt_spinningflag. When the promise resolves later, it halts the next spin, which returns immediately. rclcpp and rclpy do not behave this way.This PR gives each spin its own halt flag.
ExecutorCommands::halt_spinning()still stops the current spin, and a leftover watcher can only affect the spin that created it. There are no public API changes.Tests:
test_stale_promise_does_not_halt_later_spin: onmain, the second spin returns after ~22 µs instead of 500 mstest_halt_spinning_stops_current_spinThis pull request was assisted by Claude:claude-opus-5-5.