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

add boost::mutex::scoped_lock before accessing rt_active_goal_ #325

Closed
wants to merge 1 commit into from

Conversation

k-okada
Copy link
Contributor

@k-okada k-okada commented Mar 30, 2018

No description provided.

@bmagyar
Copy link
Member

bmagyar commented Mar 30, 2018

I'm still getting a crash, although this seems to be different than before (that being a simple segmentation fault).

Usually after a couple of seconds of running the script from #301 with the ur_gazebo setup.

gzserver: /usr/include/boost/smart_ptr/shared_ptr.hpp:648: typename boost::detail::sp_member_access<T>::type boost::shared_ptr<T>::operator->() const [with T = realtime_tools::RealtimeServerGoalHandle<control_msgs::FollowJointTrajectoryAction_<std::allocator<void> > >; typename boost::detail::sp_member_access<T>::type = realtime_tools::RealtimeServerGoalHandle<control_msgs::FollowJointTrajectoryAction_<std::allocator<void> > >*]: Assertion `px != 0' failed.
Aborted (core dumped)
[gazebo-2] process has died [pid 4104, exit code 134, cmd /opt/ros/kinetic/lib/gazebo_ros/gzserver -e ode worlds/empty.world __name:=gazebo __log:=/home/bence/.ros/log/04a85136-33ef-11e8-999d-e8b1fc66403b/gazebo-2.log].
log file: /home/bence/.ros/log/04a85136-33ef-11e8-999d-e8b1fc66403b/gazebo-2*.log

What's your experience with the same test? More debug info from me soon!

@k-okada
Copy link
Contributor Author

k-okada commented Mar 30, 2018 via email

@bmagyar
Copy link
Member

bmagyar commented Mar 30, 2018

Now it hangs Gazebo entirely: time gets stuck when I send the new goal.
I'm experiencing the same both in release and debug modes. The first message hangs time already.

@k-okada
Copy link
Contributor Author

k-okada commented Mar 30, 2018

that's strange , I had similar situation when I lock entire goalCB function, but 8925e70 works ok for me with #301 (comment) example

can you remove
8925e70#diff-9ec3f32075624b226ce54cd89fd6a606R631
line and try again.

@bmagyar
Copy link
Member

bmagyar commented Mar 30, 2018

It probably had to do with a build cache...I've tried in a new workspace and I'm experiencing no crashes now.

@mathias-luedtke
Copy link
Contributor

Is this patch RT-safe?

@bmagyar
Copy link
Member

bmagyar commented Apr 1, 2018

Good to bring that up again.
I'd think it is, but I think it's sensible to further look into the implications of using boost::scoped_lock vs the common m.try_lock() pattern we have in controllers.

For ref, I found some scoped_lock reference in realtime_tools::RealtimeBuffer and realtime_tools::RealtimeBox too. RealtimeBuffer has an interesting mix of usage of scoped_lock and m.try_lock().

@mathias-luedtke
Copy link
Contributor

The non-RT can lock unconditionally (it might block), the RT part should not block.
In addition locks might lead to syscalls.

RealtimeBuffer has an interesting mix of usage of scoped_lock and m.try_lock().

try-locks and scope-lock can be even mixed:
boost::mutex::scoped_lock lock(mutex, boost::try_to_lock);

Copy link
Contributor

@mathias-luedtke mathias-luedtke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we should add this update-wide lock.
#301 can be fixed without.
rt_active_goal_ should never get accessed directly, but from a local copy of the shared pointer.

@7675t
Copy link
Contributor

7675t commented Apr 2, 2018

Hi, do you mean we should just do as #327 ? @ipa-mdl
At a glance of the code, it uses boost::shared_ptr as a kind of lock-free access between threads, then we should always use a copy of the pointer in update().

@mathias-luedtke
Copy link
Contributor

Hi, do you mean we should just do as #327 ? @ipa-mdl

@7675t: Thanks, this is what I meant 👍

@bmagyar
Copy link
Member

bmagyar commented Apr 20, 2018

Closing in favour of #327

@bmagyar bmagyar closed this Apr 20, 2018
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.

4 participants