gazebosim / gazebo-classic

Gazebo classic. For the latest version, see https://github.com/gazebosim/gz-sim
http://classic.gazebosim.org/
Other
1.18k stars 479 forks source link

Sensor destructor are never called after Model removal #1112

Closed osrf-migration closed 10 years ago

osrf-migration commented 10 years ago

Original report (archived issue) by Silvio Traversaro (Bitbucket: traversaro).


Apparently (tried on current default with some prints) Joint (and Link) destructors methods are never called when the corresponding Model is removed from the simulation.

Is this the intended behaviour?

We became aware of this issue because we have some sensor plugins that opened some connections, and we had crashes when exiting.

Apparently the sensor plugins that are child of Link elements close correctly because they are removed in the Link::Fini() method [1] that is properly called, while the sensor plugins that are child of Joint elements don't close correctly because they should be removed in Joint destructor [2], that is never called.

If this is the intended behaviour, it is then difficult to handle connection in joint-sensor plugins, because there is no place in which to close the opened connections.

[1] : https://github.com/osrf/gazebo/blob/d216922f71925550dd3ad6465f2a982eaeae5756/gazebo/physics/Link.cc#L256

[2] : https://github.com/osrf/gazebo/blob/d216922f71925550dd3ad6465f2a982eaeae5756/gazebo/physics/Joint.cc#L81

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


The constructors should be called, but a workaround could be to create a Joint::Fini method and copy the sensor removal loop into that function.

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


I tried this locally and it seem to work (even if I am not too confident about this fix because I have not the clear idea of where the Joint::Fini method is called).

osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


Do you have a patch for what you tried?

osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


Can you try the changes in 5fefb31881f8a4a0baf23f7921006ab1730ac0cf (branch issue_1112)?

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


Yes, my modification where similar to the one in https://osrf-migration.github.io/gazebo-gh-pages/#!/osrf/gazebo/commits/1f323a1cfca4 (5fefb31881f8a4a0baf23f7921006ab1730ac0cf) . I am not in lab/home right now, I soon I get back at home I will try, thanks!

osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


I would like to create a regression test. If you can help me write a test that fails without this modification I would appreciate it.

osrf-migration commented 10 years ago

Original comment by Francesco Romano (Bitbucket: francesco_romano).


Sorry.. I replied to the email.. And I cannot remove the comment..

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


I created a plugin (basically a stripped down version of our plugin in https://github.com/robotology/gazebo_yarp_plugins without the middleware specific code) that causes a crash in gazebo from the default branch if a model including it is removed:

https://bitbucket.org/traversaro/gazebo_forcetorque_removal_test

A sample model is included in the repo for quick testing of the plugin.

Using branch issue_1112 the model is removed in a clean way.

I am not familiar with your test infastructure, let me know if I can provide additional support.

osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


We use gtest, and there are numerous tests in the test/integration and test/regression folder as examples. I would put your example plugin in the test/plugins folder, and create a world file in test/worlds that contains your test model.

There is already a test that removes models that could be duplicated with your test model. See test/integration/world.cc. We have had some trouble with this test in the past (#1042), though it may be unrelated to this issue.

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


Test for this issue added in PR https://osrf-migration.github.io/gazebo-gh-pages/#!/osrf/gazebo/pull-request/1028/add-test-for-issue-1112 .

osrf-migration commented 10 years ago

Original comment by Ian Chen (Bitbucket: Ian Chen, GitHub: iche033).


looks like it's resolved by pull request #1028

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


While pull request #1028 solve the relative problem of sensor destructor that are not called upon joint removal, the joint and link destructor are still never called after model removal, even if this is not the intended behavior (am I right @scpeters ? ).

osrf-migration commented 10 years ago

Original comment by Steve Peters (Bitbucket: Steven Peters, GitHub: scpeters).


@traversaro That may be the case; I haven't verified it. I wonder if we have some reference loops with our shared pointers?

Since the pull request fixed a real problem, perhaps we can retitle this issue and leave it closed, and file a new issue about the destructors not being called. We should write a test for that as well.

osrf-migration commented 10 years ago

Original comment by Silvio Traversaro (Bitbucket: traversaro).


osrf-migration commented 8 years ago

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).