Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Waitset not triggering timer when initialized in lifecycle node #2652

Open
luca-della-vedova opened this issue Oct 16, 2024 · 4 comments
Open
Assignees

Comments

@luca-della-vedova
Copy link
Contributor

luca-della-vedova commented Oct 16, 2024

Bug report

Required Info:

  • Operating System:
    • Ubuntu 24.04
  • Installation type:
    • Jazzy binaries (but reproducible from source, bisected to the commit linked below which introduced the issue)
  • Version or commit hash:
    • I bisected and found the exact commit that introduced this behavior (it was OK before): 5632a09
  • DDS implementation:
    • Cyclone
  • Client library (if applicable):
    • rclcpp

Steps to reproduce issue

ros2 lifecycle set /minimal_timer configure
ros2 lifecycle set /minimal_timer activate
ros2 run examples_rclcpp_minimal_service service_main

Expected behavior

Since the service callback destroys only timer_, timer2_ should keep running and Hello, world! should keep printing.

Actual behavior

All timers are stopped and no Hello, world! is printed.

Additional information

The following might shed some light into this.

  • If the timer that is erronously not triggered is initialized in the constructor, rather than in a lifecycle transition, the issue does not arise.
  • If the action server is ran before the lifecycle transition (which means timer2_ is only created after timer_ is destroyed) the issue does not arise.
  • If I manually add a || true here making this condition always evaluate to true, all timers are executed, however I added some printing when timers are cleaned up / added and I can see that every iteration a timer gets added and removed, so something odd is going on there.

So I suspect there is something about how timers (and maybe other entities?) are added to wait sets inside lifecycle node transitions that creates this issue.

@mjcarroll
Copy link
Member

This definitely sounds like a bug.

If I manually add a || true here making this condition always evaluate to true,

This is going to force the waitset to always be rebuilt, which is not good for performance. It's likely that the bug is somewhere else (in how timers are added?)

I probably won't get a chance to look at this until after roscon, but thank you for the full repro example and detailed steps.

@alsora
Copy link
Collaborator

alsora commented Oct 16, 2024

Yes, I agree that this sounds like a bug.

So I suspect there is something about how timers (and maybe other entities?) are added to wait sets inside lifecycle node transitions that creates this issue.

I wouldn't exclude that this is "timers-specific" since timers are handled in a slightly different ways from other entities.
Moreover, lifecycle nodes do not do anything special here, so the same problem should be reproducible in a regular node (executors don't really care about nodes, they only care about callback groups)

@mjcarroll
Copy link
Member

lifecycle nodes do not do anything special here

My assumption as well is that something about the execution order from lifecycle nodes is surfacing this, but it's not something that would be unique to lifecycle nodes.

@fmrico
Copy link
Contributor

fmrico commented Oct 17, 2024

Hi,

I discovered this bug five days ago at fmrico/cascade_lifecycle#14, and now I am solving a problem that is directly related at PlanSys2 (ros/rosdistro#43212 (comment)). I am solving this now by changing the SingleThreadedExecutors to EventsExecutors in some cases and removing the node before destroying it in other cases.

Basically, if I have two nodes in an executor, each one with a timer, if I destroy one of the nodes, the timer callback in the other node is no longer called.

I can investigate more if you want.

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

No branches or pull requests

4 participants