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

resetting subscriber leads to crash #349

Closed
Karsten1987 opened this issue Aug 4, 2017 · 3 comments
Closed

resetting subscriber leads to crash #349

Karsten1987 opened this issue Aug 4, 2017 · 3 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Karsten1987
Copy link
Contributor

Karsten1987 commented Aug 4, 2017

given the following code: https://gist.github.com/Karsten1987/82837b87fd11ef23353754b9fa31cf7a

the situation:

1.) node spawns a subscriber and a publisher
2.1) main thread spins the executor, having sub and pub talking
2.2) second thread randomly stops the subscription (resetting the shared_ptr of it)
3.) program crashes with the backtrace at the bottom of the gist.

quickly discussed the problem with @wjwwood, when resetting the subscription, the executor still has a raw handle on it which leads to race conditions. The problem can be addressed by turning the handle into a shared pointer.

concretely speaking, this function should return a shared_ptr instead of a raw pointer.

@Karsten1987 Karsten1987 added the bug Something isn't working label Aug 4, 2017
@Karsten1987 Karsten1987 self-assigned this Aug 4, 2017
@dhood
Copy link
Member

dhood commented Aug 23, 2017

this is the permalink to the function @Karsten1987 is referencing in his post (in case those lines move around):

RCLCPP_PUBLIC
const rcl_subscription_t *
get_subscription_handle() const;

deng02 added a commit to clearpathrobotics/rclcpp that referenced this issue Jan 16, 2018
objects as shared pointers (ros2#349)

To prevent an object from being deleted while the rcl_wait_set is
using raw pointers to internal members, we store them as shared
pointers.

The subscriptions are stored as a pair because a single
subscription handle can have both an intra and non-intra
process handle being used by the wait set.  In order to
remove objects based on null wait set handles, we need both.
@deng02
Copy link
Contributor

deng02 commented Jan 16, 2018

We have been hitting this issue when using the ROS2-to-ROS1 ros1_bridge with bridge_all_2to1_topics set to false. When the ROS1 subscriber for a topic disappers the ROS2 subscriber is destroyed and sometimes this happens while the wait set is still holding a raw pointer to a member of that object. The result is various segfault errors within rmw_wait.

guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 5, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 6, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 6, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 6, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
@Karsten1987 Karsten1987 added this to the bouncy milestone Feb 26, 2018
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 26, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Feb 27, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Mar 2, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Mar 7, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Mar 10, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
wjwwood pushed a commit that referenced this issue Mar 12, 2018
* Convert all rcl_*_t types to shared pointers

Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: #349

* fixup! Convert all rcl_*_t types to shared pointers

* fix { use on function definitions

We always put the { on a new line for function definitions and class declarations.
@wjwwood
Copy link
Member

wjwwood commented Mar 12, 2018

I believe #431 closes this, please comment here if this is still an issue.

@wjwwood wjwwood closed this as completed Mar 12, 2018
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Mar 15, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
guillaumeautran added a commit to clearpathrobotics/rclcpp that referenced this issue Mar 16, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: ros2#349
wjwwood pushed a commit that referenced this issue Mar 19, 2018
Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: #349
wjwwood added a commit that referenced this issue Mar 20, 2018
* Revert "Revert "Store the subscriber, client, service and timer (#431)" (#448)"

This reverts commit 168d75c.

* Convert all rcl_*_t types to shared pointers

Converts all rcl_*_t types in the memory allocation strategy to shared pointers to prevent crash happening when a subscriber is reset.

Issue: #349

* fixups
nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
Signed-off-by: William Woodall <william@osrfoundation.org>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this issue Aug 5, 2022
* Test rosbag2_transport for all rmw implementations

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants