gazebosim / gazebo-classic

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

gazebo does not destroy plugins in the reverse order w.r.t. to which they have been created #3192

Closed traversaro closed 2 years ago

traversaro commented 2 years ago

A common pattern in computer science in general is to destroy resources in the opposite value in which they have been created, i.e. if A and B are constructed/allocated/started in this order, destroy/deallocate/free/stop first B and then A, so that if there is any dependency of B on A, you do not risk that B crashes as A has been destroyed before.

According to a few experiments, this is not the case for many plugins in Gazebo (while for other it is, depending on where they are). In particular, for plugins belonging to the same model Gazebo stores the plugin shared pointers in Model::plugins data structure, see:

However during model destruction, there is no attempt to destroy the plugin in reverse order w.r.t. to loading order:

I can provide a patch for this behaviour, however note that touching the destruction order could have unintended consequences, as it seems a behaviour that is quite prone to Hyrum's Law .

traversaro commented 2 years ago

Downstream issue: https://github.com/robotology/gazebo-yarp-plugins/issues/582 .

traversaro commented 2 years ago

In https://github.com/traversaro/gazebo/tree/fixReverseOrder I added a tests (for now without assertions) that showcase this problem. It loads a model like:

    <model name="parent_model">
      <plugin name="plugin1" filename="libReverseDestructionOrderTestPlugin.so" />
      <model name="nested_model">
        <link name="nested_link" />
        <plugin name="plugin2" filename="libReverseDestructionOrderTestPlugin.so" />
        <plugin name="plugin3" filename="libReverseDestructionOrderTestPlugin.so" />
      </model>
    </model>

And the load/destruction patterns are:

685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:45] Load: plugin1
685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:45] Load: plugin2
685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:45] Load: plugin3
685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:37] Destructor: plugin2
685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:37] Destructor: plugin3
685: [Dbg] [ReverseDestructionOrderTestPlugin.cc:37] Destructor: plugin1

Note that the order is not the same in which the plugin are loaded, nor the reverse one.

I will be happy to provide a PR for fixing this, but I would prefer to have before the discussion on whater such a PR could be accepted or if such change is exclued a priori as it is too risky. fyi @scpeters

traversaro commented 2 years ago

I will be happy to provide a PR for fixing this, but I would prefer to have before the discussion on whater such a PR could be accepted or if such change is exclued a priori as it is too risky. fyi @scpeters

The issue is much more complex, as there is a similar issue regarding the order in which links and joints (and consequently their sensor and the related plugins) are created and destructed w.r.t. to their models. For my specific issue (https://github.com/robotology/gazebo-yarp-plugins/issues/582) I found another solution to manage explicitly the order in which I call the cleanup sequence of the plugins, so I will close this issue.