Problems when creating/destructing participant in others static objects [8987]

[ivanpauno]

In ros2, we're migrating from creating one Participant per Node to one per Context (see ros2/rmw_fastrtps#312).
The default context has for example static storage.
The destructor of that object, ends up calling: https://github.com/eProsima/Fast-RTPS/blob/a181a67b94e88839755e83a52ec72af4c9145ac6/include/fastrtps/Domain.h#L145
As the Domain object is a singleton, and destruction order of static objects in different transitional units can't be ensured, it sometimes crash (because it's used an already destructed Domain).

I didn't try, but the same bug can happen with https://github.com/eProsima/Fast-RTPS/blob/master/include/fastdds/dds/domain/DomainParticipantFactory.hpp.
And AFAIK, the same problem is common in other DDS vendors.

Expected Behavior

Have a way to construct/destroy Participants in objects with static storage.

Current Behavior

It's not possible.

Steps to Reproduce

I don't have a simple repro ....
I can share detailed steps if needed.

System information

• Fast-RTPS version: 1.9.x
• OS: Ubuntu 18.04
• Network interfaces:
  
• ROS2: Eloquent

Additional context

The only solution that I have in mind, is to provide a way to the user to create a Domain (or DomainParticipantFactory). In that way, the object can be stored in another transitional unit and destruction order can be ensured. The solution to the commented ROS 2 problem is still complicated, but possible if that's available.
We're also considering to stop having a default context, but I just want to rise awareness to the issue.

[JLBuenoLopez]

I am going to move this issue to the Ideas discussion forum. According to the CONTRIBUTING guidelines feature requests and improvements are discussed in that forum.

[maloel]

Hi,

We're experiencing the same issue:

• We have a global participant that tries to delete_participant() on exit
• The eProsima participant factory uses a singleton that is destroyed on exit
• The factory dtor also destroys all participants, even if someone is still holding them!
• If the factory dtor happens first (as can happen, since this behavior is not well-defined), then our delete_participant() call will crash

I don't really see a way around this, given the current APIs.

I don't understand why eProsima decided to go in this direction: if the factory is a singleton, why not assume that an application is well-behaved and will destroy all its objects properly? If I may suggest: destroy all the impls as you do today -- but don't destroy the participants. Instead, reset all participants' impl_ to nullptr and check it in all the participant APIs (today it just automatically forwards all calls to it).

Thanks.

P.S.: I opened PR #2739 which can help with this

[JLBuenoLopez]

Hi @maloel,

DomainParticipantFactory is defined as a singleton in the PIM (Platform Independent Model) API as per the DDS specification, specifically in the 2.2.2.2.2 clause. Therefore, Fast DDS is only complying with the specification.

Regarding the suggestion of leaving the DDS entity's destruction to the user: although the DDS PIM API returns the DDS object, the ownership is not pass to the user, but it is kept in Fast DDS. Therefore, the application architecture must ensure that the DomainParticipants are deleted before the factory is destroyed. Otherwise, memory will leak because the factory destructor only destroys the DomainParticipantImpl. Once the factory is destroyed, it is not possible to access its API without instantiating a new singleton which no longer has access to the DDS entities created with the previous singleton. So it is not possible to destroy a DomainParticipant once the factory has been destroyed.

Also, if the memory has been reserved by Fast DDS, it makes sense that Fast DDS takes charge of freeing it. Leaving the responsibility to the user is not advisable and assuming that an application is well-behaved is sometimes a large assumption.

This is a well-known limitation of the DDS PIM API that requires that the user application follows the correct destruction order.

[maloel]

Hi @JLBuenoLopez-eProsima

I wasn't suggesting that the user be responsible for freeing the memory. But the specification does define a delete_participant operation for the user to use when a participant is no longer needed, yes? Surely you're not suggesting that our code not try to delete a participant once it's not needed?

This operation crashes on destruction with the eProsima implementation, and I don't see that in the specification anywhere.

Your insistence that the user application follow a certain destruction order is impossible to implement, as that order is undefined between different translation-units.

At the current state of your code, it is impossible to know whether the participant (or really, the factory) is alive or not. Our code can only blindly delete_participant and hope for the best. Why can't the eProsima implementation guard against this scenario?

[MiguelCompany]

@maloel

I would say that the following code would ensure the factory is destroyed after your global object, if it is the only place where DomainParticipantFactory::get_instance() is called.

struct GlobalObject
{
    GlobalObject()
    {
        dds_factory_ = DomainParticipantFactory::get_instance();
        dds_participant_ = dds_factory_->create_participant(....);
    }

    ~GlobalObject()
    {
        dds_factory_->delete_participant(dds_participant_);
    }

private:
    DomainParticipantFactory* dds_factory_ = nullptr;
    DomainParticipant* dds_participant_ = nullptr;
};

If you cannot architect your application that way, we could refactor DomainParticipantFactory to have a std::shared_ptr<DomainParticipantFactory> get_shared_instance() method, and then change get_instance() to just do a return get_shared_instance().get().

Thoughts?

[maloel]

@MiguelCompany
Miguel, hi :)

We tried your approach (GlobalObject) with the same results: the factory is destroyed before ~GlobalObject() and delete_participant() crashes. In fact, because the GlobalObject ctor happens before any of the eProsima ctors, the call to DomainParticipantFactory::get_instance() itself crashes!

What you're suggesting with the shared_ptr is a step in the right direction: we need to avoid cases where factory destruction will make participant actions crash. I.e., a participant pointer may still be held post-destruction of the factory, so its actions must return an error or do nothing. What I did in the PR is to have the destructor invalidate any live participants rather than delete them (they still get deleted on delete_participant), but it is only one way to go about it.

Regards,
-Eran

[MiguelCompany]

@maloel Would you mind attaching a simple reproducer?

I just checked with a simple application with the following code, and had no crashes (checked both Windows and Linux).

struct GlobalObject
{
    GlobalObject()
    {
        std::cout << "Constructing GlobalObject" << std::endl;
        dds_factory = eprosima::fastdds::dds::DomainParticipantFactory::get_instance();
        dds_participant = dds_factory->create_participant(0, eprosima::fastdds::dds::PARTICIPANT_QOS_DEFAULT);
    }

    ~GlobalObject()
    {
        std::cout << "Destroying GlobalObject" << std::endl;
        dds_factory->delete_participant(dds_participant);
    }

    eprosima::fastdds::dds::DomainParticipantFactory* dds_factory;
    eprosima::fastdds::dds::DomainParticipant* dds_participant;
};

static GlobalObject obj;

int main()
{
    std::cout << "Exiting main" << std::endl;
    return 0;
}

[maloel]

@MiguelCompany
Unfortunately our usage isn't simple :) : the GlobalObject is in a library that's linking with FastDDS statically, and then used in another application. That's probably what's causing the linker to choose some weird initialization/destruction order.
If really needed, I can maybe provide a branch that does this but it'll take work on my time and yours to actually compile and run it. If you want to pursue this further let's take it offline?

[ivanpauno]

I would say that the following code would ensure the factory is destroyed after your global object, if it is the only place where DomainParticipantFactory::get_instance() is called.

No, that's not the case.
That only ensures that the DomainParticipantFactory is constructed in its first use (get_instance()), but it doesn't ensure that it's destructed after ~GlobalObject() (*).
Destruction order of static objects cannot be guaranteed (the nifty counter pattern is a workarround for that, but it's not practical except when you're the maintainer of all the translation units).

Would you mind attaching a simple reproducer?

Destruction order issues of objects is not only platform dependent, but a small change in code can randomly make it work again or not.
In my experience, it's pretty hard to get a mimimal reproducer of these kind of issues.

---

(*) IIRC, it's not guaranteed (in all platforms) that destructors are called in reverse order than constructors for objects of static lifetime in different translations units (particularly, I think windows breaks that, though I'm not completely sure).
And even if that would be guaranteed, there are two options in the code you shared:

• DomainParticipantFactory() is constructed before the line eprosima::fastdds::dds::DomainParticipantFactory::get_instance().
  In that case the order would be DomainParticipantFactory(), GlobalObject(), ~GlobalObject(), ~DomainParticipantFactory().
• DomainParticipantFactory() is constructed in the line eprosima::fastdds::dds::DomainParticipantFactory::get_instance().
  In that case the order would be GlobalObject(), DomainParticipantFactory(), ~DomainParticipantFactory(), ~GlobalObject(), which is a problem.

---

Something more usable than the "nifty counter" pattern is:

// can be used if you want to guarantee destruction order
std::shared_ptr<DomainParticipantFactory>
DomainParticipantFactory::get_instance_shared() {
  static auto g = std::make_shared<DomainParticipantFactory>();
  return g;
}

// cannot be used if you want to guarantee destruction order
DomainParticipantFactory *
DomainParticipantFactory::get_instance() {
  return get_instance_shared().get();
}

struct GlobalObject
{
    GlobalObject()
    {
        std::cout << "Constructing GlobalObject" << std::endl;
        dds_factory = eprosima::fastdds::dds::DomainParticipantFactory::get_instance_shared();
        dds_participant = dds_factory->create_participant(0, eprosima::fastdds::dds::PARTICIPANT_QOS_DEFAULT);
    }

    ~GlobalObject()
    {
        std::cout << "Destroying GlobalObject" << std::endl;
        dds_factory->delete_participant(dds_participant);
    }

    std::shared_ptr<eprosima::fastdds::dds::DomainParticipantFactory> dds_factory;
    eprosima::fastdds::dds::DomainParticipant* dds_participant;
};

That guarantees both construction and destruction order, even if GlobalObject and DomainParticipant are in different translation units.

---

Better than the above option IMO, is to allow users to create a DomainParticipantFactory() with non-static lifetime.
Even if it doesn't make sense or it's a problem to construct multiple, allowing users to manage the lifetime of it is a good idea, considering all cpp construction/destruction order quirks.

[MiguelCompany]

@maloel @ivanpauno I have just created #2894 adding DomainParticipantFactory::get_shared_instance()

[maloel]

@maloel @ivanpauno I have just created #2894 adding DomainParticipantFactory::get_shared_instance()

Hi @MiguelCompany,

As also mentioned elsewhere, I figured I'd document here:

The addition of DomainParticipantFactory::get_shared_instance() is aimed at helping prolong lifetime until all client objects (participant, mainly) are done with it.

I just tried it and, while it acts as intended, we're still getting crashes on exit, though elsewhere:

The factory contains references to an RTPS participant which is already destroyed. It calls RTPSDomain::removeRTPSParticipant(rtps_participant) which crashes when it tries to logError.

There seem to be other low-level static objects in the system that are still a problem.

[MiguelCompany]

@maloel Please check PR #3185 where some low-level static objects are being removed