Simplify service manager's tracking of singletons

It only tracks whether to dispose a singleton instance now, and (at least
conceptually) no longer remembers the single instance (apart from what is
necessary in order to call dispose on it), as the underlying implementation
already needs to keep track of that to support direct calls of constructor
functions.

Change-Id: I154bf05438e1db099c1c5ffb1c56377725c6bfc6

cppuhelper/source/servicemanager.cxx

@@ -701,43 +701,19 @@ cppuhelper::ServiceManager::Data::Implementation::createInstance(
     css::uno::Reference<css::uno::XComponentContext> const & context,
     bool singletonRequest)
 {
-    if (info->singletons.empty()) {
-        assert(!singletonRequest);
+    css::uno::Reference<css::uno::XInterface> inst;
     if (constructor != 0) {
-            // the interface is already acquired, don't acquire here
-            return css::uno::Reference<css::uno::XInterface>(
-                (*constructor)(context.get(),
-                    css::uno::Sequence<css::uno::Any>()),
-                SAL_NO_ACQUIRE);
-        }
-        if (factory1.is()) {
-            return factory1->createInstanceWithContext(context);
-        }
-        assert(factory2.is());
-        return factory2->createInstance();
-    } else {
-        osl::MutexGuard g(mutex); //TODO: must be a non-recursive mutex
-        if (singleton.is()) {
-            if (singletonRequest) {
-                dispose = false;
-            }
-            return singleton;
-        }
-        if (constructor != 0) {
-            // the interface is already acquired, don't acquire here
-            singleton.set(
-                (*constructor)(context.get(),
-                    css::uno::Sequence<css::uno::Any>()),
+        inst.set(
+            (*constructor)(context.get(), css::uno::Sequence<css::uno::Any>()),
             SAL_NO_ACQUIRE);
     } else if (factory1.is()) {
-            singleton = factory1->createInstanceWithContext(context);
+            inst = factory1->createInstanceWithContext(context);
     } else {
         assert(factory2.is());
-            singleton = factory2->createInstance();
-        }
-        dispose = singleton.is() && !singletonRequest;
-        return singleton;
+        inst = factory2->createInstance();
     }
+    updateDisposeSingleton(singletonRequest, inst);
+    return inst;
 }
 
 css::uno::Reference<css::uno::XInterface>
@@ -745,65 +721,54 @@ cppuhelper::ServiceManager::Data::Implementation::createInstanceWithArguments(
     css::uno::Reference<css::uno::XComponentContext> const & context,
     bool singletonRequest, css::uno::Sequence<css::uno::Any> const & arguments)
 {
-    if (info->singletons.empty()) {
-        assert(!singletonRequest);
+    css::uno::Reference<css::uno::XInterface> inst;
     if (constructor != 0) {
-            // the interface is already acquired, don't acquire here
-            css::uno::Reference<css::uno::XInterface> inst(
-                (*constructor)(context.get(), arguments), SAL_NO_ACQUIRE);
-
-            // HACK: The service can implement XInitialization too.
-            // It should be removed when converting to constructor-based
-            // initialization, but in case somebody forgets, be safe, and
-            // call it anyway for the moment.
-            css::uno::Reference<css::lang::XInitialization> xinit(
+        inst.set((*constructor)(context.get(), arguments), SAL_NO_ACQUIRE);
+        //HACK: The constructor will either observe arguments and return inst
+        // that does not implement XInitialization (or null), or ignore
+        // arguments and return inst that implements XInitialization; this
+        // should be removed again once XInitialization-based implementations
+        // have become rare:
+        css::uno::Reference<css::lang::XInitialization> init(
             inst, css::uno::UNO_QUERY);
-            if (xinit.is()) {
-                xinit->initialize(arguments);
+        if (init.is()) {
+            init->initialize(arguments);
         }
         return inst;
-        }
-        if (factory1.is()) {
-            return factory1->createInstanceWithArgumentsAndContext(
+    } else if (factory1.is()) {
+        inst = factory1->createInstanceWithArgumentsAndContext(
             arguments, context);
-        }
-        assert(factory2.is());
-        return factory2->createInstanceWithArguments(arguments);
     } else {
-        osl::MutexGuard g(mutex); //TODO: must be a non-recursive mutex
-        if (singleton.is()) {
-            SAL_WARN(
-                "cppuhelper",
-                "createInstanceWithArguments request for already instantiated"
-                    " singleton implementation " << info->name);
+        assert(factory2.is());
+        inst = factory2->createInstanceWithArguments(arguments);
+    }
+    updateDisposeSingleton(singletonRequest, inst);
+    return inst;
+}
+
+void cppuhelper::ServiceManager::Data::Implementation::updateDisposeSingleton(
+    bool singletonRequest,
+    css::uno::Reference<css::uno::XInterface> const & instance)
+{
+    // This is an optimization, to only call dispose once (from the component
+    // context) on a singleton that is obtained both via the component context
+    // and via the service manager; however, there is a harmless race here that
+    // may cause two calls to dispose nevertheless (also, this calls dispose on
+    // at most one of the instances obtained via the service manager, in case
+    // the implementation hands out different instances):
     if (singletonRequest) {
+        osl::MutexGuard g(mutex);
+        disposeSingleton.clear();
         dispose = false;
+    } else if (!info->singletons.empty()) {
+        css::uno::Reference<css::lang::XComponent> comp(
+            instance, css::uno::UNO_QUERY);
+        if (comp.is()) {
+            osl::MutexGuard g(mutex);
+            if (dispose) {
+                disposeSingleton = comp;
             }
-            return singleton;
-        }
-        if (constructor != 0) {
-            // the interface is already acquired, don't acquire here
-            singleton.set(
-                (*constructor)(context.get(), arguments), SAL_NO_ACQUIRE);
-
-            // HACK: The service can implement XInitialization too.
-            // It should be removed when converting to constructor-based
-            // initialization, but in case somebody forgets, be safe, and
-            // call it anyway for the moment.
-            css::uno::Reference<css::lang::XInitialization> xinit(
-                singleton, css::uno::UNO_QUERY);
-            if (xinit.is()) {
-                xinit->initialize(arguments);
-            }
-        } else if (factory1.is()) {
-            singleton = factory1->createInstanceWithArgumentsAndContext(
-                arguments, context);
-        } else {
-            assert(factory2.is());
-            singleton = factory2->createInstanceWithArguments(arguments);
         }
-        dispose = singleton.is() && !singletonRequest;
-        return singleton;
     }
 }
 
@@ -938,7 +903,7 @@ void cppuhelper::ServiceManager::loadImplementation(
 }
 
 void cppuhelper::ServiceManager::disposing() {
-    std::vector< css::uno::Reference<css::uno::XInterface> > sngls;
+    std::vector< css::uno::Reference<css::lang::XComponent> > sngls;
     std::vector< css::uno::Reference< css::lang::XComponent > > comps;
     Data clear;
     {
@@ -950,8 +915,8 @@ void cppuhelper::ServiceManager::disposing() {
             assert(i->second.get() != 0);
             if (!i->second->info->singletons.empty()) {
                 osl::MutexGuard g2(i->second->mutex);
-                if (i->second->dispose) {
-                    sngls.push_back(i->second->singleton);
+                if (i->second->disposeSingleton.is()) {
+                    sngls.push_back(i->second->disposeSingleton);
                 }
             }
         }
@@ -962,8 +927,8 @@ void cppuhelper::ServiceManager::disposing() {
             assert(i->second.get() != 0);
             if (!i->second->info->singletons.empty()) {
                 osl::MutexGuard g2(i->second->mutex);
-                if (i->second->dispose) {
-                    sngls.push_back(i->second->singleton);
+                if (i->second->disposeSingleton.is()) {
+                    sngls.push_back(i->second->disposeSingleton);
                 }
             }
             if (i->second->component.is()) {
@@ -976,15 +941,12 @@ void cppuhelper::ServiceManager::disposing() {
         data_.singletons.swap(clear.singletons);
     }
     for (std::vector<
-             css::uno::Reference<css::uno::XInterface> >::const_iterator i(
+             css::uno::Reference<css::lang::XComponent> >::const_iterator i(
                  sngls.begin());
          i != sngls.end(); ++i)
     {
-        css::uno::Reference<css::lang::XComponent> comp(
-            *i, css::uno::UNO_QUERY);
-        if (comp.is()) {
         try {
-                comp->dispose();
+            (*i)->dispose();
         } catch (css::uno::RuntimeException & e) {
             SAL_WARN(
                 "cppuhelper",
@@ -992,7 +954,6 @@ void cppuhelper::ServiceManager::disposing() {
                     << "\" while disposing singleton");
         }
     }
-    }
     for (std::vector<
              css::uno::Reference< css::lang::XComponent > >::const_iterator i(
                  comps.begin());

cppuhelper/source/servicemanager.hxx

@@ -106,8 +106,7 @@ public:
                     new ImplementationInfo(
                         name, loader, uri, environment, constructorName, prefix,
                         alienContext, rdbFile)),
-                constructor(0), status(STATUS_NEW),
-                dispose(false)
+                constructor(0), status(STATUS_NEW), dispose(true)
             {}
 
             Implementation(
@@ -120,8 +119,7 @@ public:
                     theComponent):
                 info(new ImplementationInfo(name)), constructor(0),
                 factory1(theFactory1), factory2(theFactory2),
-                component(theComponent), status(STATUS_LOADED),
-                dispose(false)
+                component(theComponent), status(STATUS_LOADED), dispose(true)
             { assert(theFactory1.is() || theFactory2.is()); }
 
             css::uno::Reference<css::uno::XInterface> createInstance(
@@ -146,8 +144,13 @@ public:
             Status status;
 
             osl::Mutex mutex;
-            css::uno::Reference<css::uno::XInterface> singleton;
+            css::uno::Reference< css::lang::XComponent > disposeSingleton;
             bool dispose;
+
+        private:
+            void updateDisposeSingleton(
+                bool singletonRequest,
+                css::uno::Reference<css::uno::XInterface> const & instance);
         };
 
         typedef std::map< rtl::OUString, boost::shared_ptr< Implementation > >