Fixes #840: removes manual reference counting (#841)

* converted registration * compiled, not passeD * fixed weak_ptr error, now leaking mock objects * fixed bug, need to decrement ref count * updats from last week, test cases failing because of out of date expectations * all tests are passing with shared and weak pointers to serviceRegistrationBasePrivate * still passing tests, updated comments and cleaned up * updating to share dependents * new issue with dying service * added coreInfo, maybe passing * passing tests, removed manual ref counting from referenceBasePrivate, repeated all fast tests to ensure no sporadic failures * updated comments * updates before PR * Removed manual reference counting The manual reference counting in ServiceRegistrationBasePrivate and ServiceReferenceBasePrivate were removed. Additionally, some properties of ServiceRegistrationBasePrivate were offloaded to a new class ServiceRegistrationCoreInfo which both ServiceRegistrationBasePrivate and ServiceReferenceBasePrivate can access allowing ServiceReferenceBasePrivate to give up ownership of ServiceRegistrationBasePrivate. Signed-off-by: Toby Cormack <tcormack@mathworks.com> * Removed manual reference counting and merged with upstream (#840) The manual reference counting in ServiceRegistrationBasePrivate and ServiceReferenceBasePrivate were removed. Additionally, some properties of ServiceRegistrationBasePrivate were offloaded to a new class ServiceRegistrationCoreInfo which both ServiceRegistrationBasePrivate and ServiceReferenceBasePrivate can access allowing ServiceReferenceBasePrivate to give up ownership of ServiceRegistrationBasePrivate. Signed-off-by: Toby Cormack <tcormack@mathworks.com> * ServiceRegistrationCoreInfo now default destructor * Updated based on Patty's comments #840 * updated ServiceReferenceBase Constructors for clarity with shared_ptrs * removed 'move' from serviceRegistry * attempt at solving mac issue * updates for lock type and removing unneccessary functions from reference * lost lock * changed to custom atomic load * LockSet addition * threading support in LockSet * LockSet not threaded * no names in func dec * Incoorporated Jeff's Comments * Assignment operator didn't fail on my computer, did in github * updated for Jeff's 5/16 comments * clang update * mikes comments and fixes for multithreaded support * remove ifdefs from BundleRegistry, abide by rule of (0,3,5), and add comments --------- Signed-off-by: Toby Cormack <tcormack@mathworks.com>
CppMicroServices · May 19, 2023 · 062fd47 · 062fd47 · github-actions · May 19, 2023
1 parent d43df28
commit 062fd47
Show file tree

Hide file tree

Showing 25 changed files with 584 additions and 369 deletions.
diff --git a/.clang-format b/.clang-format
@@ -33,6 +33,7 @@ Cpp11BracedListStyle: false
 IndentCaseLabels: true
 IndentGotoLabels: false
 IndentPPDirectives: AfterHash
+InsertBraces: true
 NamespaceIndentation: All
 PackConstructorInitializers: CurrentLine
 PointerAlignment: Left

diff --git a/.gitignore b/.gitignore
@@ -40,4 +40,4 @@ doc/src/examples/makefile/main
 *.pyc
 /build*
 .vscode/
-.venv/
+.venv/
diff --git a/framework/include/cppmicroservices/ServiceReferenceBase.h b/framework/include/cppmicroservices/ServiceReferenceBase.h
@@ -24,6 +24,7 @@
 #define CPPMICROSERVICES_SERVICEREFERENCEBASE_H
 
 #include "cppmicroservices/Any.h"
+#include "cppmicroservices/detail/Threads.h"
 #include <functional>
 
 #include <atomic>
@@ -232,13 +233,16 @@ namespace cppmicroservices
          */
         ServiceReferenceBase();
 
-        ServiceReferenceBase(ServiceRegistrationBasePrivate* reg);
+        ServiceReferenceBase(std::shared_ptr<ServiceRegistrationBasePrivate> reg);
 
         void SetInterfaceId(std::string const& interfaceId);
 
         // This class is not thread-safe, but we support thread-safe
-        // copying and assignment.
-        std::atomic<ServiceReferenceBasePrivate*> d;
+        // copying and assignment
+        // This was changed to a std::shared_ptr and is accessed through
+        // Load(), but when this repository uses c++20,
+        // this will be changed to std::atomic<shared_ptr>
+        cppmicroservices::detail::Atomic<std::shared_ptr<ServiceReferenceBasePrivate>> d;
     };
 
     /**

diff --git a/framework/include/cppmicroservices/ServiceRegistrationBase.h b/framework/include/cppmicroservices/ServiceRegistrationBase.h
@@ -1,22 +1,22 @@
- /*=============================================================================
+/*=============================================================================
 
-  Library: CppMicroServices
+ Library: CppMicroServices
 
-  Copyright (c) The CppMicroServices developers. See the COPYRIGHT
-  file at the top-level directory of this distribution and at
-  https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
+ Copyright (c) The CppMicroServices developers. See the COPYRIGHT
+ file at the top-level directory of this distribution and at
+ https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
 
-  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
+ 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
+   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.
+ 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.
 
 =============================================================================*/
 
@@ -30,6 +30,7 @@
 namespace cppmicroservices
 {
 
+    class ServiceRegistrationLocks;
     class BundlePrivate;
     class ServiceRegistrationBasePrivate;
     class Properties;
@@ -196,11 +197,13 @@ namespace cppmicroservices
          */
         ServiceRegistrationBase();
 
-        ServiceRegistrationBase(ServiceRegistrationBasePrivate* registrationPrivate);
+        ServiceRegistrationBase(std::shared_ptr<ServiceRegistrationBasePrivate> registrationPrivate);
 
         ServiceRegistrationBase(BundlePrivate* bundle, InterfaceMapConstPtr const& service, Properties&& props);
 
-        ServiceRegistrationBasePrivate* d { nullptr };
+        std::shared_ptr<ServiceRegistrationLocks> LockServiceRegistration() const;
+
+        std::shared_ptr<ServiceRegistrationBasePrivate> d;
     };
 
     /**
@@ -224,7 +227,7 @@ namespace cppmicroservices
  */
 
 US_HASH_FUNCTION_BEGIN(cppmicroservices::ServiceRegistrationBase)
-return std::hash<cppmicroservices::ServiceRegistrationBasePrivate*>()(arg.d);
+return std::hash<std::shared_ptr<cppmicroservices::ServiceRegistrationBasePrivate>>()(arg.d);
 US_HASH_FUNCTION_END
 
 #endif // CPPMICROSERVICES_SERVICEREGISTRATIONBASE_H
diff --git a/framework/include/cppmicroservices/detail/Threads.h b/framework/include/cppmicroservices/detail/Threads.h
@@ -106,6 +106,7 @@ namespace cppmicroservices
                 UniqueLock&
                 operator=(UniqueLock&&)
                 {
+                    return *this;
                 }
                 explicit UniqueLock(MutexLockingStrategy const&) {}
                 explicit UniqueLock(MutexLockingStrategy const*) {}

diff --git a/framework/src/CMakeLists.txt b/framework/src/CMakeLists.txt
@@ -19,6 +19,7 @@ set(_srcs
   util/SharedLibrary.cpp
   util/SharedLibraryException.cpp
   util/Utils.cpp
+  util/ServiceRegistrationLocks.cpp
 
   service/ListenerToken.cpp
   service/ServiceException.cpp
@@ -34,6 +35,7 @@ set(_srcs
   service/ServiceReferenceBasePrivate.cpp
   service/ServiceRegistrationBase.cpp
   service/ServiceRegistrationBasePrivate.cpp
+  service/ServiceRegistrationCoreInfo.cpp
   service/ServiceRegistry.cpp
 
   bundle/Bundle.cpp
@@ -67,13 +69,15 @@ set(_private_headers
   util/Properties.h
   util/PropsCheck.h
   util/Utils.h
+  util/ServiceRegistrationLocks.h
 
   service/ServiceHooks.h
   service/ServiceListenerEntry.h
   service/ServiceListenerHookPrivate.h
   service/ServiceListeners.h
   service/ServiceReferenceBasePrivate.h
   service/ServiceRegistrationBasePrivate.h
+  service/ServiceRegistrationCoreInfo.h
   service/ServiceRegistry.h
 
   bundle/BundleArchive.h

diff --git a/framework/src/bundle/BundleContext.cpp b/framework/src/bundle/BundleContext.cpp
@@ -1,22 +1,22 @@
- /*=============================================================================
+/*=============================================================================
 
-  Library: CppMicroServices
+ Library: CppMicroServices
 
-  Copyright (c) The CppMicroServices developers. See the COPYRIGHT
-  file at the top-level directory of this distribution and at
-  https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
+ Copyright (c) The CppMicroServices developers. See the COPYRIGHT
+ file at the top-level directory of this distribution and at
+ https://github.com/CppMicroServices/CppMicroServices/COPYRIGHT .
 
-  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
+ 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
+   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.
+ 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.
 
 =============================================================================*/
 
@@ -274,8 +274,7 @@ namespace cppmicroservices
         {
             try
             {
-                sref.d.load()->UngetService(b.lock(), true);
-                //sref.d.load()->UngetService(b.lock(), true);
+                sref.d.Load()->UngetService(b.lock(), true);
             }
             catch (...)
             {
@@ -312,7 +311,7 @@ namespace cppmicroservices
         auto b = GetAndCheckBundlePrivate(d);
 
         std::shared_ptr<ServiceHolder<void>> h(
-            new ServiceHolder<void>(b, reference, reference.d.load()->GetService(b.get())));
+            new ServiceHolder<void>(b, reference, reference.d.Load()->GetService(b.get())));
         return std::shared_ptr<void>(h, h->service.get());
     }
 
@@ -333,7 +332,7 @@ namespace cppmicroservices
         d->CheckValid();
         auto b = GetAndCheckBundlePrivate(d);
 
-        auto serviceInterfaceMap = reference.d.load()->GetServiceInterfaceMap(b.get());
+        auto serviceInterfaceMap = reference.d.Load()->GetServiceInterfaceMap(b.get());
         std::shared_ptr<ServiceHolder<InterfaceMap const>> h(
             new ServiceHolder<InterfaceMap const>(b, reference, serviceInterfaceMap));
         return InterfaceMapConstPtr(h, h->service.get());

diff --git a/framework/src/bundle/BundleHooks.cpp b/framework/src/bundle/BundleHooks.cpp
@@ -74,7 +74,7 @@ namespace cppmicroservices
         {
             ServiceReference<BundleFindHook> sr = srBaseIter->GetReference();
             std::shared_ptr<BundleFindHook> fh
-                = std::static_pointer_cast<BundleFindHook>(sr.d.load()->GetService(GetPrivate(selfBundle).get()));
+                = std::static_pointer_cast<BundleFindHook>(sr.d.Load()->GetService(GetPrivate(selfBundle).get()));
             if (fh)
             {
                 try
@@ -139,7 +139,7 @@ namespace cppmicroservices
                 }
 
                 std::shared_ptr<BundleEventHook> eh = std::static_pointer_cast<BundleEventHook>(
-                    sr.d.load()->GetService(GetPrivate(GetBundleContext().GetBundle()).get()));
+                    sr.d.Load()->GetService(GetPrivate(GetBundleContext().GetBundle()).get()));
                 if (eh)
                 {
                     try

diff --git a/framework/src/bundle/BundlePrivate.cpp b/framework/src/bundle/BundlePrivate.cpp
@@ -384,7 +384,9 @@ namespace cppmicroservices
                         try
                         {
                             if (util::Exists(bundleDir))
+                            {
                                 util::RemoveDirectoryRecursive(bundleDir);
+                            }
                         }
                         catch (...)
                         {
@@ -823,7 +825,8 @@ namespace cppmicroservices
         coreCtx->services.GetUsedByBundle(this, srs);
         for (std::vector<ServiceRegistrationBase>::const_iterator i = srs.begin(); i != srs.end(); ++i)
         {
-            i->GetReference(std::string()).d.load()->UngetService(this->shared_from_this(), false);
+            auto ref = i->GetReference(std::string());
+            ref.d.Load()->UngetService(this->shared_from_this(), false);
         }
     }
 

diff --git a/framework/src/service/ServiceHooks.cpp b/framework/src/service/ServiceHooks.cpp
@@ -125,7 +125,7 @@ namespace cppmicroservices
             {
                 ServiceReference<ServiceFindHook> sr = fhrIter->GetReference();
                 auto fh
-                    = std::static_pointer_cast<ServiceFindHook>(sr.d.load()->GetService(GetPrivate(selfBundle).get()));
+                    = std::static_pointer_cast<ServiceFindHook>(sr.d.Load()->GetService(GetPrivate(selfBundle).get()));
                 if (fh)
                 {
                     try
@@ -177,7 +177,7 @@ namespace cppmicroservices
             {
                 ServiceReference<ServiceEventListenerHook> sr = sriIter->GetReference();
                 auto elh = std::static_pointer_cast<ServiceEventListenerHook>(
-                    sr.d.load()->GetService(GetPrivate(selfBundle).get()));
+                    sr.d.Load()->GetService(GetPrivate(selfBundle).get()));
                 if (elh)
                 {
                     try

diff --git a/framework/src/service/ServiceListeners.cpp b/framework/src/service/ServiceListeners.cpp
@@ -460,7 +460,7 @@ namespace cppmicroservices
         // Get a copy of the service reference and keep it until we are
         // done with its properties.
         auto ref = evt.GetServiceReference();
-        auto props = ref.d.load()->GetProperties();
+        auto props = ref.d.Load()->GetProperties();
 
         {
             auto l = this->Lock();
@@ -469,7 +469,9 @@ namespace cppmicroservices
             for (auto& sse : complicatedListeners)
             {
                 if (receivers.count(sse) == 0)
+                {
                     continue;
+                }
                 LDAPExpr const& ldapExpr = sse.GetLDAPExpr();
                 if (ldapExpr.IsNull() || ldapExpr.Evaluate(props, false))
                 {

diff --git a/framework/src/service/ServiceObjects.cpp b/framework/src/service/ServiceObjects.cpp
@@ -60,11 +60,11 @@ namespace cppmicroservices
 
             if (isPrototypeScope)
             {
-                result = m_reference.d.load()->GetPrototypeService(MakeBundleContext(m_context).GetBundle());
+                result = m_reference.d.Load()->GetPrototypeService(MakeBundleContext(m_context).GetBundle());
             }
             else
             {
-                result = m_reference.d.load()->GetServiceInterfaceMap(
+                result = m_reference.d.Load()->GetServiceInterfaceMap(
                     GetPrivate(MakeBundleContext(m_context).GetBundle()).get());
             }
 
@@ -115,11 +115,11 @@ namespace cppmicroservices
 
                     if (isPrototypeScope)
                     {
-                        sref.d.load()->UngetPrototypeService(bundle, interfaceMap);
+                        sref.d.Load()->UngetPrototypeService(bundle, interfaceMap);
                     }
                     else
                     {
-                        sref.d.load()->UngetService(bundle, true);
+                        sref.d.Load()->UngetService(bundle, true);
                     }
                 }
             }