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

LifecycleNode shutdown on dtor only with valid context. #2545

Merged
merged 1 commit into from
May 30, 2024

Conversation

fujitatomoya
Copy link
Collaborator

@fujitatomoya fujitatomoya commented May 30, 2024

#2528 generates the following error when the context is gracefully shutdown. (e.g deferred signal handler of rclcpp.)

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run lifecycle lifecycle_talker
^C[INFO] [1717052032.575390927] [rclcpp]: signal_handler(signum=2)
[ERROR] [1717052032.577706294] [lc_talker]: Unable to start transition 5 from current state shuttingdown: Could not publish transition: publisher's context is invalid, at /root/ros2_ws/colcon_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:423, at /root/ros2_ws/colcon_ws/src/ros2/rcl/rcl_lifecycle/src/rcl_lifecycle.c:368
[WARN] [1717052032.577764887] [rclcpp_lifecycle]: Shutdown error in destruction of LifecycleNode: final state(unconfigured)

this is because the shutdown in the dtor is called but failed to publish the transition since the context is not valid.

i thought of a couple of things, but at this moment, i would like to label it with TODO.

  • add the context shutdown callback with weak_ptr lifecyclenode. so that even with graceful shutdown, context shutdown callback will be calling the lifecyclenode::shutdown properly.
  • add shutdown without publishing the transition. lifecycnode itself needs to be shutdown to avoid leaving the device or sensor in unknown state. we can call this method when the context is invalid.

This also addresses #2547 (Nav2 CI finds this behavior)

@fujitatomoya
Copy link
Collaborator Author

@mjcarroll @clalancette can you take a look at this?

@fujitatomoya
Copy link
Collaborator Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator Author

CI is green, i will go ahead to merge this.

@fujitatomoya fujitatomoya merged commit d8d83a0 into rolling May 30, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fujitatomoya/lc-node-shutdown-dtor-fix branch May 30, 2024 21:57
fujitatomoya added a commit that referenced this pull request May 30, 2024
fujitatomoya added a commit that referenced this pull request May 30, 2024
fujitatomoya added a commit that referenced this pull request May 30, 2024
fujitatomoya added a commit that referenced this pull request May 30, 2024
fujitatomoya added a commit that referenced this pull request Jun 6, 2024
…2544)

* lifecycle node dtor shutdown should be called only in primary state.

Signed-off-by: Tomoya Fujita <[email protected]>

* LifecycleNode shutdown on dtor only with valid context. (#2545)

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
fujitatomoya added a commit that referenced this pull request Jun 6, 2024
…2543)

* lifecycle node dtor shutdown should be called only in primary state.

Signed-off-by: Tomoya Fujita <[email protected]>

* LifecycleNode shutdown on dtor only with valid context. (#2545)

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
fujitatomoya added a commit that referenced this pull request Jun 7, 2024
…known state (2nd) (backport #2528) (#2542)

* call shutdown in LifecycleNode dtor to avoid leaving the device in unknown state (2nd) (#2528)

* Revert "Revert "call shutdown in LifecycleNode dtor to avoid leaving the device in un… (#2450)" (#2522)"

This reverts commit 42b0b57.

Signed-off-by: Tomoya Fujita <[email protected]>

* lifecycle node dtor shutdown should be called only in primary state.

Signed-off-by: Tomoya Fujita <[email protected]>

* adjust warning message if the node is still in transition state in dtor.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
(cherry picked from commit 3bc364a)

* LifecycleNode shutdown on dtor only with valid context. (#2545)

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
Co-authored-by: Tomoya Fujita <[email protected]>
fujitatomoya added a commit that referenced this pull request Jun 10, 2024
fujitatomoya added a commit that referenced this pull request Jun 11, 2024
* Revert "LifecycleNode shutdown on dtor only with valid context. (#2545)"

This reverts commit d8d83a0.

Signed-off-by: Tomoya Fujita <[email protected]>

* Revert "call shutdown in LifecycleNode dtor to avoid leaving the device in unknown state (2nd) (#2528)"

This reverts commit 3bc364a.

Signed-off-by: Tomoya Fujita <[email protected]>

---------

Signed-off-by: Tomoya Fujita <[email protected]>
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.

2 participants