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

2 files changed, 76 insertions, 112 deletions

diff --git a/cppuhelper/source/servicemanager.cxx b/cppuhelper/source/servicemanager.cxx
index e6c31fc53302..973201d0f3c2 100644
--- a/cppuhelper/source/servicemanager.cxx
+++ b/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);
-        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();
+    css::uno::Reference<css::uno::XInterface> inst;
+    if (constructor != 0) {
+        inst.set(
+            (*constructor)(context.get(), css::uno::Sequence<css::uno::Any>()),
+            SAL_NO_ACQUIRE);
+    } else if (factory1.is()) {
+            inst = factory1->createInstanceWithContext(context);
     } 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>()),
-                SAL_NO_ACQUIRE);
-        } else if (factory1.is()) {
-            singleton = factory1->createInstanceWithContext(context);
-        } else {
-            assert(factory2.is());
-            singleton = factory2->createInstance();
-        }
-        dispose = singleton.is() && !singletonRequest;
-        return singleton;
+        assert(factory2.is());
+        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);
-        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, css::uno::UNO_QUERY);
-            if (xinit.is()) {
-                xinit->initialize(arguments);
-            }
-            return inst;
-        }
-        if (factory1.is()) {
-            return factory1->createInstanceWithArgumentsAndContext(
-                arguments, context);
+    css::uno::Reference<css::uno::XInterface> inst;
+    if (constructor != 0) {
+        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 (init.is()) {
+            init->initialize(arguments);
         }
-        assert(factory2.is());
-        return factory2->createInstanceWithArguments(arguments);
+        return inst;
+    } else if (factory1.is()) {
+        inst = factory1->createInstanceWithArgumentsAndContext(
+            arguments, context);
     } 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);
-            if (singletonRequest) {
-                dispose = false;
-            }
-            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);
+        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;
             }
-        } 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,21 +941,17 @@ 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();
-            } catch (css::uno::RuntimeException & e) {
-                SAL_WARN(
-                    "cppuhelper",
-                    "Ignoring RuntimeException \"" << e.Message
-                        << "\" while disposing singleton");
-            }
+        try {
+            (*i)->dispose();
+        } catch (css::uno::RuntimeException & e) {
+            SAL_WARN(
+                "cppuhelper",
+                "Ignoring RuntimeException \"" << e.Message
+                    << "\" while disposing singleton");
         }
     }
     for (std::vector<
diff --git a/cppuhelper/source/servicemanager.hxx b/cppuhelper/source/servicemanager.hxx
index b6e506bfbd47..1e760ca78310 100644
--- a/cppuhelper/source/servicemanager.hxx
+++ b/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 > >