Fix race condition on Publisher shutdown

[iche033]

The race condition problem occurs when the Publisher::OnPublishComplete callback is invoked from a different thread after the Publisher object is destroyed, causing a crash. There's been a few attempts to fixing this race condition, see this commit and this PR, but the issue persists and it is just harder to reproduce. When successfully reproduced, I get a assertion error in this line when locking the mutex.

The fix is to bind the callback function along with a shared pointer instead of raw pointer this so that the object is kept alive for a little longer until after the callback is complete. In the case of Publisher, this should be safe to do as the OnPublishComplete function is mainly just doing some bookkeeping work. Note that I added a PublisherPrivate class and stored instances of this class in a static variable to avoid breaking ABI.

To reproduce this issue:

1. Save this model as box.sdf file:

<?xml version="1.0" ?>
<sdf version="1.6">
  <model name="box">
    <pose>0 0 0.5 0 0 0</pose>
    <link name="link">
      <collision name="collision">
        <geometry>
          <box>
            <size>1 1 1</size>
          </box>
        </geometry>
      </collision>
      <visual name="visual">
        <geometry>
          <box>
            <size>1 1 1</size>
          </box>
        </geometry>
      </visual>
    </link>
  </model>
</sdf>

2. launch gazebo using roslaunch:

roslaunch gazebo_ros empty_world.launch verbose:=true

3. Run the following script that spawns and deletes the box model 500 times:

#!/bin/bash
for i in {0..500}; do
    echo "$i"
    rosparam set pizza --textfile box.sdf
    rosrun gazebo_ros spawn_model -param pizza -sdf -model box -x 0 -y 0.5 -z 0
    sleep 0.5
    rosservice call gazebo/delete_model '{model_name: box}'
done

4. Without changes in this PR, the crash occurs on spawn after a couple hundred iterations for me

Signed-off-by: Ian Chen [email protected]

[chapulina]

Wow, nice ABI gymnastics!

The fix is to bind the callback function along with a shared pointer instead of raw pointer this so that the object is kept alive for a little longer

Interesting, in this case I guess the global static map helped, because if you had used a regular dataPtr as unique_ptr, it would have died together with the Publisher.

[chapulina]

Could use the opportunity to change to std::bind

[iche033]

done. c1e40a5

[chapulina]

I remember having a reason to destroy all publishers before the node, but I can't remember exactly why. Looking through the code, this pattern seems common:

this->pub.reset();
this->node->Fini();
this->node.reset();

If you don't have a strong reason for terminating the publisher after the node, I'd recommend switching order here.

[iche033]

moved erase before resetting node c1e40a5

[iche033]

Interesting, in this case I guess the global static map helped, because if you had used a regular dataPtr as unique_ptr, it would have died together with the Publisher.

yep the publisher object has to be a shared ptr in a static map. I added a comment in ce2b9df

[chapulina]

The latest updates look good too 👍