robotology / gz-sim-yarp-plugins

YARP plugins for Modern Gazebo (gz-sim).
BSD 3-Clause "New" or "Revised" License
9 stars 2 forks source link

Fix robotinterface plugin crashes by closing it as soon as one of its devices is destroyed #186

Closed traversaro closed 3 months ago

traversaro commented 3 months ago

Fix https://github.com/robotology/gz-sim-yarp-plugins/issues/185 .

As the destroying order of the Gazedo entities is not reversed, it occurred that the robotinterface plugin crashed as it was still calling devices that were already destroyed. This PR fixes this problem by closing the robotinterface plugin as soon as one of the devices to which it is attached is destroyed. Basically this is a port of https://github.com/robotology/gazebo-yarp-plugins/pull/619 to gz-sim-yarp-plugins.

xela-95 commented 3 months ago

Thanks a lot @traversaro!

In the corresponding PR for gazebo-yarp-plugins https://github.com/robotology/gazebo-yarp-plugins/pull/619#issuecomment-1086682037 I've seen that to check that the implementation was working as expected, you tried to remove and re-add the model without crashes. Have you tried the same here to check the implementation? We can maybe add some test to spot potential future regressions?

traversaro commented 3 months ago

Thanks a lot @traversaro!

In the corresponding PR for gazebo-yarp-plugins robotology/gazebo-yarp-plugins#619 (comment) I've seen that to check that the implementation was working as expected, you tried to remove and re-add the model without crashes. Have you tried the same here to check the implementation? We can maybe add some test to spot potential future regressions?

The problem is that the problem occurs as a result of a race condition between the NWS period threads and gazebo threads. However, the test that we suppressed in https://github.com/robotology/gz-sim-yarp-plugins/pull/179 pass fine with this fix, so that is a sort of test for this.

xela-95 commented 3 months ago

Just because I've mentioned it before, in gz-sim has been added the equivalent of the Gazebo Classic insert tab (for drag-and-drop of models), it is called Resource Spawner, see https://github.com/gazebosim/gz-sim/pull/173#issuecomment-658485569

xela-95 commented 3 months ago

Rebased on main.

xela-95 commented 3 months ago

Merging 🚀