diff --git a/include/ignition/gazebo/components/Component.hh b/include/ignition/gazebo/components/Component.hh index 59eee24fb6..af28be1f9c 100644 --- a/include/ignition/gazebo/components/Component.hh +++ b/include/ignition/gazebo/components/Component.hh @@ -387,6 +387,8 @@ namespace components /// \brief Unique name for this component type. This is set through the /// Factory registration. + // TODO(azeey) Change to const char* in Harmonic to prevent static + // initialization order fiasco. public: inline static std::string typeName; }; @@ -433,6 +435,8 @@ namespace components /// \brief Unique name for this component type. This is set through the /// Factory registration. + // TODO(azeey) Change to const char* in Harmonic to prevent static + // initialization order fiasco. public: inline static std::string typeName; }; diff --git a/include/ignition/gazebo/components/Factory.hh b/include/ignition/gazebo/components/Factory.hh index 5e280a906c..3d9d0d31b2 100644 --- a/include/ignition/gazebo/components/Factory.hh +++ b/include/ignition/gazebo/components/Factory.hh @@ -19,9 +19,11 @@ #include #include +#include #include #include #include +#include #include #include @@ -110,10 +112,135 @@ namespace components } }; + /// \brief A wrapper around uintptr_t to prevent implicit conversions. + struct RegistrationObjectId + { + /// \brief Construct object from a pointer. + /// \param[in] _ptr Arbitrary pointer. + explicit RegistrationObjectId(void *_ptr) + : id(reinterpret_cast(_ptr)) + { + } + + /// \brief Construct object from a uintptr_t. + /// \param[in] _ptr Arbitrary pointer address. + explicit RegistrationObjectId(std::uintptr_t _ptrAddress) + : id(_ptrAddress) + { + } + + /// \brief Equality comparison. + /// \param[in] _other Other RegistrationObjectId object to compare against. + bool operator==(const RegistrationObjectId &_other) const + { + return this->id == _other.id; + } + + /// \brief Wrapped uintptr_t variable. + std::uintptr_t id; + }; + + + /// \brief A class to hold the queue of component descriptors registered by + /// translation units. This queue is necessary to ensure that component + /// creation continues to work after plugins are unloaded. The typical + /// scenario this aims to solve is: + /// 1. Plugin P1 registers component descripter for component C1. + /// 2. Plugin P1 gets unloaded. + /// 3. Plugin P2 registers a component descriptor for component C1 and tries + /// to create an instance of C1. + /// When P1 gets unloaded, the destructor of the static component + /// registration object calls Factory::Unregister which removes the component + /// descriptor from the queue. Without this step, P2 would attempt to use + /// the component descriptor created by P1 in step 3 and likely segfault + /// because the memory associated with that descriptor has been deleted when + /// P1 was unloaded. + class ComponentDescriptorQueue + { + /// \brief Check if the queue is empty + public: bool IGNITION_GAZEBO_HIDDEN Empty() + { + return this->queue.empty(); + } + + /// \brief Add a component descriptor to the queue + /// \param[in] _regObjId An ID that identifies the registration object. This + /// is generally derived from the `this` pointer of the static component + /// registration object created when calling IGN_GAZEBO_REGISTER_COMPONENT. + /// \param[in] _comp The component descriptor + public: void IGNITION_GAZEBO_HIDDEN Add(RegistrationObjectId _regObjId, + ComponentDescriptorBase *_comp) + { + this->queue.push_front({_regObjId, _comp}); + } + + /// \brief Remove a component descriptor from the queue. This also deletes + /// memory allocated for the component descriptor by the static component + /// registration object. + /// \param[in] _regObjId An ID that identifies the registration object. This + /// is generally derived from the `this` pointer of the static component + /// registration object created when calling IGN_GAZEBO_REGISTER_COMPONENT. + public: void IGNITION_GAZEBO_HIDDEN Remove(RegistrationObjectId _regObjId) + { + auto compIt = std::find_if(this->queue.rbegin(), this->queue.rend(), + [&](const auto &_item) + { return _item.first == _regObjId; }); + + if (compIt != this->queue.rend()) + { + ComponentDescriptorBase *compDesc = compIt->second; + this->queue.erase(std::prev(compIt.base())); + delete compDesc; + } + } + + /// \brief Create a component using the latest available component + /// descriptor. This simply forward to ComponentDescriptorBase::Create + /// \sa ComponentDescriptorBase::Create + public: IGNITION_GAZEBO_HIDDEN std::unique_ptr Create() const + { + if (!this->queue.empty()) + { + return this->queue.front().second->Create(); + } + return {}; + } + + /// \brief Create a component using the latest available component + /// descriptor. This simply forward to ComponentDescriptorBase::Create + /// \sa ComponentDescriptorBase::Create + public: IGNITION_GAZEBO_HIDDEN std::unique_ptr Create( + const components::BaseComponent *_data) const + { + if (!this->queue.empty()) + { + return this->queue.front().second->Create(_data); + } + return {}; + } + + /// \brief Queue of component descriptors registered by static registration + /// objects. + private: std::deque> queue; + }; + /// \brief A factory that generates a component based on a string type. + // TODO(azeey) Do not inherit from common::SingletonT in Harmonic class Factory : public ignition::common::SingletonT { + // Deleted copy constructors to make the ABI checker happy + public: Factory(Factory &) = delete; + public: Factory(const Factory &) = delete; + // Since the copy constructors are deleted, we need to explicitly declare a + // default constructor. + // TODO(azeey) Make this private in Harmonic + public: Factory() = default; + + /// \brief Get an instance of the singleton + public: IGNITION_GAZEBO_VISIBLE static Factory *Instance(); + /// \brief Register a component so that the factory can create instances /// of the component and its storage based on an ID. /// \param[in] _type Type of component to register. @@ -136,16 +263,27 @@ namespace components /// \param[in] _compDesc Object to manage the creation of ComponentTypeT /// objects. /// \tparam ComponentTypeT Type of component to register. - public: template + // TODO(azeey) Deprecate in favor of overload that takes _regObjId + public: template void Register(const std::string &_type, ComponentDescriptorBase *_compDesc) { - // Every time a plugin which uses a component type is loaded, it attempts - // to register it again, so we skip it. - if (ComponentTypeT::typeId != 0) - { - return; - } + this->Register(_type, _compDesc, + RegistrationObjectId{nullptr}); + } + /// \brief Register a component so that the factory can create instances + /// of the component based on an ID. + /// \param[in] _type Type of component to register. + /// \param[in] _compDesc Object to manage the creation of ComponentTypeT + /// objects. + /// \param[in] _regObjId An ID that identifies the registration object. This + /// is generally derived from the `this` pointer of the static component + /// registration object created when calling IGN_GAZEBO_REGISTER_COMPONENT. + /// \tparam ComponentTypeT Type of component to register. + public: template + void Register(const std::string &_type, ComponentDescriptorBase *_compDesc, + RegistrationObjectId _regObjId) + { auto typeHash = ignition::common::hash64(_type); // Initialize static member variable - we need to set these @@ -169,8 +307,8 @@ namespace components << runtimeNameIt->second << "] and type [" << runtimeName << "] with name [" << _type << "]. Second type will not work." << std::endl; + return; } - return; } // This happens at static initialization time, so we can't use common @@ -184,7 +322,7 @@ namespace components } // Keep track of all types - this->compsById[ComponentTypeT::typeId] = _compDesc; + this->compsById[ComponentTypeT::typeId].Add(_regObjId, _compDesc); namesById[ComponentTypeT::typeId] = ComponentTypeT::typeName; runtimeNamesById[ComponentTypeT::typeId] = runtimeName; } @@ -192,12 +330,23 @@ namespace components /// \brief Unregister a component so that the factory can't create instances /// of the component anymore. /// \tparam ComponentTypeT Type of component to unregister. - public: template + // TODO(azeey) Deprecate in favor of overload that takes _regObjId + public: template void Unregister() { - this->Unregister(ComponentTypeT::typeId); + this->Unregister(RegistrationObjectId{nullptr}); + } - ComponentTypeT::typeId = 0; + /// \brief Unregister a component so that the factory can't create instances + /// of the component anymore. + /// \tparam ComponentTypeT Type of component to unregister. + /// \param[in] _regObjId An ID that identifies the registration object. This + /// is generally derived from the `this` pointer of the static component + /// registration object created when calling IGN_GAZEBO_REGISTER_COMPONENT. + public: template + void Unregister(RegistrationObjectId _regObjId) + { + this->Unregister(ComponentTypeT::typeId, _regObjId); } /// \brief Unregister a component so that the factory can't create instances @@ -206,36 +355,32 @@ namespace components /// within the component type itself. Prefer using the templated /// `Unregister` function when possible. /// \param[in] _typeId Type of component to unregister. + // TODO(azeey) Deprecate in favor of overload that takes _regObjId public: void Unregister(ComponentTypeId _typeId) { - // Not registered - if (_typeId == 0) - { - return; - } - - { - auto it = this->compsById.find(_typeId); - if (it != this->compsById.end()) - { - delete it->second; - this->compsById.erase(it); - } - } + this->Unregister(_typeId, RegistrationObjectId{nullptr}); + } + /// \brief Unregister a component so that the factory can't create instances + /// of the component anymore. + /// \details This function will not reset the `typeId` static variable + /// within the component type itself. Prefer using the templated + /// `Unregister` function when possible. + /// \param[in] _typeId Type of component to unregister. + /// \param[in] _regObjId An ID that identifies the registration object. This + /// is generally derived from the `this` pointer of the static component + /// registration object created when calling IGN_GAZEBO_REGISTER_COMPONENT. + public: void Unregister(ComponentTypeId _typeId, + RegistrationObjectId _regObjId) + { + auto it = this->compsById.find(_typeId); + if (it != this->compsById.end()) { - auto it = namesById.find(_typeId); - if (it != namesById.end()) - { - namesById.erase(it); - } - } + it->second.Remove(_regObjId); - { - auto it = runtimeNamesById.find(_typeId); - if (it != runtimeNamesById.end()) + if (it->second.Empty()) { - runtimeNamesById.erase(it); + this->compsById.erase(it); } } } @@ -261,9 +406,10 @@ namespace components // Create a new component if a FactoryFn has been assigned to this type. std::unique_ptr comp; auto it = this->compsById.find(_type); - if (it != this->compsById.end() && nullptr != it->second) - comp = it->second->Create(); - + if (it != this->compsById.end()) + { + comp = it->second.Create(); + } return comp; } @@ -291,8 +437,10 @@ namespace components else { auto it = this->compsById.find(_type); - if (it != this->compsById.end() && nullptr != it->second) - comp = it->second->Create(_data); + if (it != this->compsById.end()) + { + comp = it->second.Create(_data); + } } return comp; @@ -339,19 +487,7 @@ namespace components } /// \brief A list of registered components where the key is its id. - /// - /// Note about compsByName and compsById. The maps store pointers as the - /// values, but never cleans them up, which may (at first glance) seem like - /// incorrect behavior. This is not a mistake. Since ComponentDescriptors - /// are created at the point in the code where components are defined, this - /// generally ends up in a shared library that will be loaded at runtime. - /// - /// Because this and the plugin loader both use static variables, and the - /// order of static initialization and destruction are not guaranteed, this - /// can lead to a scenario where the shared library is unloaded (with the - /// ComponentDescriptor), but the Factory still exists. For this reason, - /// we just keep a pointer, which will dangle until the program is shutdown. - private: std::map compsById; + private: std::map compsById; /// \brief A list of IDs and their equivalent names. public: std::map namesById; @@ -375,12 +511,20 @@ namespace components { \ public: IgnGazeboComponents##_classname() \ { \ - if (_classname::typeId != 0) \ - return; \ - using namespace ignition;\ + using namespace ignition; \ using Desc = gazebo::components::ComponentDescriptor<_classname>; \ gazebo::components::Factory::Instance()->Register<_classname>(\ - _compType, new Desc());\ + _compType, new Desc(), gazebo::components::RegistrationObjectId(this));\ + } \ + public: IgnGazeboComponents##_classname( \ + const IgnGazeboComponents##_classname&) = delete; \ + public: IgnGazeboComponents##_classname( \ + IgnGazeboComponents##_classname&) = delete; \ + public: ~IgnGazeboComponents##_classname() \ + { \ + using namespace ignition; \ + gazebo::components::Factory::Instance()->Unregister<_classname>( \ + gazebo::components::RegistrationObjectId(this)); \ } \ }; \ static IgnGazeboComponents##_classname\ diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d4ea8bc402..b88261e28d 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -46,6 +46,7 @@ set (sources Barrier.cc BaseView.cc Conversions.cc + ComponentFactory.cc EntityComponentManager.cc LevelManager.cc Link.cc diff --git a/src/ComponentFactory.cc b/src/ComponentFactory.cc new file mode 100644 index 0000000000..52f5e55569 --- /dev/null +++ b/src/ComponentFactory.cc @@ -0,0 +1,25 @@ +/* + * Copyright (C) 2022 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "ignition/gazebo/components/Factory.hh" + +using Factory = ignition::gazebo::components::Factory; + +Factory *Factory::Instance() +{ + return common::SingletonT::Instance(); +} diff --git a/src/ComponentFactory_TEST.cc b/src/ComponentFactory_TEST.cc index e9adeb972a..88ef642e17 100644 --- a/src/ComponentFactory_TEST.cc +++ b/src/ComponentFactory_TEST.cc @@ -68,7 +68,7 @@ TEST_F(ComponentFactoryTest, Register) EXPECT_EQ(registeredCount + 1, ids.size()); EXPECT_NE(ids.end(), std::find(ids.begin(), ids.end(), MyCustom::typeId)); - // Fail to register same component twice + // Registering the component twice doesn't change the number of type ids. factory->Register("ign_gazebo_components.MyCustom", new components::ComponentDescriptor()); @@ -85,11 +85,8 @@ TEST_F(ComponentFactoryTest, Register) // Unregister factory->Unregister(); - // Check it has no type id yet ids = factory->TypeIds(); - EXPECT_EQ(registeredCount, ids.size()); - EXPECT_EQ(0u, MyCustom::typeId); - EXPECT_EQ("", factory->Name(MyCustom::typeId)); + EXPECT_EQ(registeredCount + 1, ids.size()); } /////////////////////////////////////////////////