Merge pull request #1926 from arcaneframework/dev/gg-simplify-memory-management-in-di

grospelliergilles · web-flow · commit 1d17e40ca97d · 2025-01-16T21:12:34.000+01:00
Simplify memory management in Dependency Injection
diff --git a/arcane/src/arcane/utils/DependencyInjection.cc b/arcane/src/arcane/utils/DependencyInjection.cc
@@ -70,7 +70,10 @@ class Injector::Impl
 
  public:
 
+  // Il faut conserver une instance de FactoryInfo pour éviter sa
+  // destruction prématurée car les instances dans m_factories en ont besoin.
   UniqueArray<Ref<impl::IInstanceFactory>> m_factories;
+  UniqueArray<impl::FactoryInfo> m_factories_info;
 };
 
 /*---------------------------------------------------------------------------*/
@@ -159,16 +162,23 @@ _doError1(const String& message, int nb_value)
 /*---------------------------------------------------------------------------*/
 /*---------------------------------------------------------------------------*/
 
-class FactoryInfo::Impl
+class FactoryInfoImpl
 {
  public:
-  Impl(const ProviderProperty& property)
+
+  FactoryInfoImpl(const ProviderProperty& property)
   : m_property(property)
   , m_name(property.name())
   {
   }
 
  public:
+
+  bool hasName(const String& str) const { return str == m_name; }
+  void fillWithImplementationNames(Array<String>& names) const { names.add(m_name); }
+
+ public:
+
   const ProviderProperty m_property;
   UniqueArray<Ref<IInstanceFactory>> m_factories;
   String m_name;
@@ -179,22 +189,13 @@ class FactoryInfo::Impl
 
 FactoryInfo::
 FactoryInfo(const ProviderProperty& property)
-: m_p{ new Impl(property) }
+: m_p(std::make_shared<FactoryInfoImpl>(property))
 {
 }
 
 /*---------------------------------------------------------------------------*/
 /*---------------------------------------------------------------------------*/
 
-FactoryInfo::
-~FactoryInfo()
-{
-  delete m_p;
-}
-
-/*---------------------------------------------------------------------------*/
-/*---------------------------------------------------------------------------*/
-
 void FactoryInfo::
 addFactory(Ref<IInstanceFactory> f)
 {
@@ -207,16 +208,7 @@ addFactory(Ref<IInstanceFactory> f)
 bool FactoryInfo::
 hasName(const String& str) const
 {
-  return str == m_p->m_name;
-}
-
-/*---------------------------------------------------------------------------*/
-/*---------------------------------------------------------------------------*/
-
-void FactoryInfo::
-fillWithImplementationNames(Array<String>& names) const
-{
-  names.add(m_p->m_name);
+  return m_p->hasName(str);
 }
 
 /*---------------------------------------------------------------------------*/
@@ -255,11 +247,11 @@ fillWithGlobalFactories()
   Integer i = 0;
   while (g) {
     auto func = g->infoCreatorWithPropertyFunction();
-    impl::FactoryInfo* fi = nullptr;
-    if (func)
-      fi = (*func)(g->property());
-    if (fi)
-      m_p->m_factories.addRange(fi->m_p->m_factories);
+    if (func) {
+      impl::FactoryInfo fi = (*func)(g->property());
+      m_p->m_factories_info.add(fi);
+      m_p->m_factories.addRange(fi.m_p->m_factories);
+    }
 
     g = g->nextRegisterer();
     ++i;
@@ -311,7 +303,7 @@ _iterateFactories(const String& factory_name, IFactoryVisitorFunctor* functor) c
     Int32 nb_constructor_arg = f->nbConstructorArg();
     if (nb_constructor_arg >= 0 && nb_constructor_arg != nb_instance)
       continue;
-    if (has_no_name || f->factoryInfo()->hasName(factory_name)) {
+    if (has_no_name || f->factoryInfoImpl()->hasName(factory_name)) {
       if (functor->execute(f))
         return;
     }
@@ -363,7 +355,7 @@ _printValidImplementationAndThrow(const TraceInfo& ti,
   for (Int32 i = 0, n = _nbFactory(); i < n; ++i) {
     impl::IInstanceFactory* f = _factory(i);
     if (filter_func(f)) {
-      f->factoryInfo()->fillWithImplementationNames(valid_names);
+      f->factoryInfoImpl()->fillWithImplementationNames(valid_names);
     }
   };
   String message = String::format("No implementation named '{0}' found", implementation_name);
diff --git a/arcane/src/arcane/utils/internal/DependencyInjection.h b/arcane/src/arcane/utils/internal/DependencyInjection.h
@@ -48,6 +48,7 @@ class Injector;
 namespace Arcane::DependencyInjection::impl
 {
 class FactoryInfo;
+class FactoryInfoImpl;
 class IInstanceFactory;
 template <typename InterfaceType>
 class IConcreteFactory;
@@ -337,12 +338,40 @@ class ARCANE_UTILS_EXPORT IInstanceFactory
  public:
 
   virtual InjectedInstanceRef createGenericReference(Injector& injector, const String& name) = 0;
+  virtual const FactoryInfoImpl* factoryInfoImpl() const = 0;
+  virtual ConcreteFactoryTypeInfo concreteFactoryInfo() const = 0;
+  virtual Int32 nbConstructorArg() const = 0;
+};
 
-  virtual const FactoryInfo* factoryInfo() const = 0;
+/*---------------------------------------------------------------------------*/
+/*---------------------------------------------------------------------------*/
+/*!
+ * \internal
+ * \brief Informations pour une fabrique.
+ */
+class ARCANE_UTILS_EXPORT FactoryInfo
+{
+  friend class Arcane::DependencyInjection::Injector;
 
-  virtual ConcreteFactoryTypeInfo concreteFactoryInfo() const = 0;
+ private:
 
-  virtual Int32 nbConstructorArg() const = 0;
+  explicit FactoryInfo(const ProviderProperty& property);
+
+ public:
+
+  static FactoryInfo create(const ProviderProperty& property, const char* file_name, int line_number)
+  {
+    ARCANE_UNUSED(file_name);
+    ARCANE_UNUSED(line_number);
+    return FactoryInfo(property);
+  }
+  void addFactory(Ref<IInstanceFactory> f);
+  bool hasName(const String& str) const;
+  const FactoryInfoImpl* _impl() const { return m_p.get(); }
+
+ private:
+
+  std::shared_ptr<FactoryInfoImpl> m_p;
 };
 
 /*---------------------------------------------------------------------------*/
@@ -367,10 +396,14 @@ template <typename InterfaceType>
 class InstanceFactory
 : public AbstractInstanceFactory
 {
+  // NOTE: On ne conserve pas une instance de 'FactoryInfo'
+  // mais uniquement son implémentation pour éviter des références croisées
+  // avec le std::shared_ptr.
+
  public:
 
-  InstanceFactory(FactoryInfo* si, IConcreteFactory<InterfaceType>* sub_factory)
-  : m_factory_info(si)
+  InstanceFactory(const FactoryInfoImpl* si, IConcreteFactory<InterfaceType>* sub_factory)
+  : m_factory_info_impl(si)
   , m_sub_factory(sub_factory)
   {
   }
@@ -390,9 +423,9 @@ class InstanceFactory
     return _createReference(injector);
   }
 
-  const FactoryInfo* factoryInfo() const override
+  const FactoryInfoImpl* factoryInfoImpl() const override
   {
-    return m_factory_info;
+    return m_factory_info_impl;
   }
 
   ConcreteFactoryTypeInfo concreteFactoryInfo() const override
@@ -407,8 +440,8 @@ class InstanceFactory
 
  protected:
 
-  FactoryInfo* m_factory_info;
-  IConcreteFactory<InterfaceType>* m_sub_factory;
+  const FactoryInfoImpl* m_factory_info_impl = nullptr;
+  IConcreteFactory<InterfaceType>* m_sub_factory = nullptr;
 
  private:
 
@@ -460,44 +493,6 @@ class IConcreteFactory
   virtual Ref<InterfaceType> createReference(Injector&) = 0;
 };
 
-/*---------------------------------------------------------------------------*/
-/*---------------------------------------------------------------------------*/
-/*!
- * \internal
- * \brief Informations pour une fabrique.
- */
-class ARCANE_UTILS_EXPORT FactoryInfo
-{
-  friend class Arcane::DependencyInjection::Injector;
-  class Impl;
-
- public:
-
-  FactoryInfo(const FactoryInfo&) = delete;
-  FactoryInfo& operator=(const FactoryInfo&) = delete;
-  ~FactoryInfo();
-
- private:
-
-  explicit FactoryInfo(const ProviderProperty& property);
-
- public:
-
-  static FactoryInfo* create(const ProviderProperty& property, const char* file_name, int line_number)
-  {
-    ARCANE_UNUSED(file_name);
-    ARCANE_UNUSED(line_number);
-    return new FactoryInfo(property);
-  }
-  void addFactory(Ref<IInstanceFactory> f);
-  bool hasName(const String& str) const;
-  void fillWithImplementationNames(Array<String>& names) const;
-
- private:
-
-  Impl* m_p = nullptr;
-};
-
 /*---------------------------------------------------------------------------*/
 /*---------------------------------------------------------------------------*/
 
@@ -516,7 +511,7 @@ class ARCANE_UTILS_EXPORT GlobalRegisterer
 
  public:
 
-  typedef FactoryInfo* (*FactoryCreateFunc)(const ProviderProperty& property);
+  typedef FactoryInfo (*FactoryCreateFunc)(const ProviderProperty& property);
 
  public:
 
@@ -914,7 +909,7 @@ class InterfaceListRegisterer
    * créer une instance de \a ConcreteType via le constructeur \a ConstructorType
    */
   template <typename ConcreteType, typename ConstructorType> void
-  registerFactory(FactoryInfo* si)
+  registerFactory(FactoryInfo& si)
   {
     _registerFactory<ConcreteType, ConstructorType, Interfaces...>(si);
   }
@@ -924,10 +919,10 @@ class InterfaceListRegisterer
   template <typename ConcreteType, typename ConstructorType,
             typename InterfaceType, typename... OtherInterfaces>
   void
-  _registerFactory(FactoryInfo* fi)
+  _registerFactory(FactoryInfo& fi)
   {
     auto* factory = new ConcreteFactory<InterfaceType, ConcreteType, ConstructorType>();
-    fi->addFactory(createRef<InstanceFactory<InterfaceType>>(fi, factory));
+    fi.addFactory(createRef<InstanceFactory<InterfaceType>>(fi._impl(), factory));
     // Applique récursivement pour les autres interfaces si nécessaire
     if constexpr (sizeof...(OtherInterfaces) > 0)
       _registerFactory<ConcreteType, ConstructorType, OtherInterfaces...>(fi);
@@ -949,7 +944,7 @@ class InjectionRegisterer
 
   //! Enregistre dans \a si les fabriques correspondentes aux constructeurs \a Constructors
   template <typename... Constructors> void
-  registerProviderInfo(FactoryInfo* si, const Constructors&... args)
+  registerProviderInfo(FactoryInfo& si, const Constructors&... args)
   {
     _create(si, args...);
   }
@@ -963,14 +958,14 @@ class InjectionRegisterer
 
   //! Surcharge pour 1 constructeur
   template <typename ConstructorType> void
-  _create(FactoryInfo* si, const ConstructorType&)
+  _create(FactoryInfo& si, const ConstructorType&)
   {
     m_interface_list.template registerFactory<ConcreteType, ConstructorType>(si);
   }
 
   //! Surcharge pour 2 constructeurs ou plus
   template <typename C1, typename C2, typename... OtherConstructors>
-  void _create(FactoryInfo* si, const C1& c1, const C2& c2, const OtherConstructors&... args)
+  void _create(FactoryInfo& si, const C1& c1, const C2& c2, const OtherConstructors&... args)
   {
     _create<C1>(si, c1);
     // Applique la récursivité sur les types restants
@@ -1000,10 +995,10 @@ class InjectionRegisterer
 #define ARCANE_DI_REGISTER_PROVIDER(t_class, t_provider_property, t_interfaces, ...) \
   namespace \
   { \
-    Arcane::DependencyInjection::impl::FactoryInfo* \
+    Arcane::DependencyInjection::impl::FactoryInfo \
     ARCANE_JOIN_WITH_LINE(arcaneCreateDependencyInjectionProviderInfo##t_class)(const Arcane::DependencyInjection::ProviderProperty& property) \
     { \
-      auto* si = Arcane::DependencyInjection::impl::FactoryInfo::create(property, __FILE__, __LINE__); \
+      auto si = Arcane::DependencyInjection::impl::FactoryInfo::create(property, __FILE__, __LINE__); \
       Arcane::DependencyInjection::impl::InjectionRegisterer<t_class, t_interfaces> injection_registerer; \
       injection_registerer.registerProviderInfo(si, __VA_ARGS__); \
       return si; \
diff --git a/arcane/src/arcane/utils/tests/TestDependencyInjection.cc b/arcane/src/arcane/utils/tests/TestDependencyInjection.cc
@@ -339,13 +339,13 @@ TEST(DependencyInjection, ConstructorCall)
 
   try {
     Injector injector;
-    IB* ib{ new BImpl() };
-    injector.bind(ib);
+    std::unique_ptr<IB> ib{ std::make_unique<BImpl>() };
+    injector.bind(ib.get());
     injector.bind(x);
     Ref<IA2> a2 = c2f.createReference(injector);
     ARCANE_CHECK_POINTER(a2.get());
     ASSERT_EQ(a2->value(), 3);
-    ASSERT_EQ(a2->bValue(), ib);
+    ASSERT_EQ(a2->bValue(), ib.get());
   }
   catch (const Exception& ex) {
     std::cerr << "ERROR=" << ex << "\n";
