-#306 Modify the logic of detach and close to not end up with double …

…locking Signed-off-by: Sumanth Nirmal <[email protected]>
eclipse-cyclonedds · Aug 23, 2022 · 47b4ac0 · 47b4ac0
1 parent efaa81f
commit 47b4ac0
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 13 deletions.
diff --git a/src/ddscxx/include/org/eclipse/cyclonedds/core/cond/ConditionDelegate.hpp b/src/ddscxx/include/org/eclipse/cyclonedds/core/cond/ConditionDelegate.hpp
@@ -66,7 +66,7 @@ class OMG_DDS_API ConditionDelegate :
 
     void init(ObjectDelegate::weak_ref_type weak_ref);
 
-    void close(const dds_entity_t entity_handle = DDS_HANDLE_NIL);
+    void close();
 
     virtual bool trigger_value() const = 0;
 
@@ -94,8 +94,8 @@ class OMG_DDS_API ConditionDelegate :
     virtual bool remove_waitset(
         org::eclipse::cyclonedds::core::cond::WaitSetDelegate *waitset);
 
-    virtual void detach_from_waitset(
-        const dds_entity_t entity_handle);
+    virtual void detach_from_waitset(const dds_entity_t entity_handle);
+    virtual void detach_and_close(const dds_entity_t entity_handle);
 
     dds::core::cond::TCondition<ConditionDelegate> wrapper();
 

diff --git a/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/ConditionDelegate.cpp b/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/ConditionDelegate.cpp
@@ -47,24 +47,33 @@ void
 org::eclipse::cyclonedds::core::cond::ConditionDelegate::detach_from_waitset(
     const dds_entity_t entity_handle) {
     std::vector<WaitSetDelegate *> waitset_list_tmp;
-    org::eclipse::cyclonedds::core::ScopedMutexLock scopedLock(this->waitSetListUpdateMutex);
+    org::eclipse::cyclonedds::core::ScopedMutexLock scopedLockForCopy(this->waitSetListUpdateMutex);
     waitset_list_tmp.assign(this->waitSetList.begin(), this->waitSetList.end());
-    scopedLock.unlock();
+    scopedLockForCopy.unlock();
 
     for (auto waitset : waitset_list_tmp) {
         org::eclipse::cyclonedds::core::ScopedObjectLock scopedWaisetLock(*waitset);
-        if (this->waitSetList.erase(waitset)) {
-            waitset->remove_condition_locked(this, entity_handle);
-       }
+        {
+            org::eclipse::cyclonedds::core::ScopedMutexLock scopedLock(this->waitSetListUpdateMutex);
+            // remove the waitset from the list and detach the condition
+            if (this->waitSetList.erase(waitset)) {
+                waitset->remove_condition_locked(this, entity_handle);
+            }
+        }
     }
 }
 
 void
-org::eclipse::cyclonedds::core::cond::ConditionDelegate::close(
+org::eclipse::cyclonedds::core::cond::ConditionDelegate::detach_and_close(
     const dds_entity_t entity_handle)
 {
-    // detach the condition from any waitsets
     detach_from_waitset(entity_handle);
+    org::eclipse::cyclonedds::core::cond::ConditionDelegate::close();
+}
+
+void
+org::eclipse::cyclonedds::core::cond::ConditionDelegate::close()
+{
     // close the condition
     DDScObjectDelegate::close();
 

diff --git a/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/GuardConditionDelegate.cpp b/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/GuardConditionDelegate.cpp
@@ -38,7 +38,7 @@ org::eclipse::cyclonedds::core::cond::GuardConditionDelegate::close()
 {
     this->check();
     org::eclipse::cyclonedds::core::ScopedObjectLock scopedLock(*this);
-    ConditionDelegate::close(ddsc_entity);
+    ConditionDelegate::detach_and_close(ddsc_entity);
 }
 
 

diff --git a/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/StatusConditionDelegate.cpp b/src/ddscxx/src/org/eclipse/cyclonedds/core/cond/StatusConditionDelegate.cpp
@@ -61,7 +61,7 @@ org::eclipse::cyclonedds::core::cond::StatusConditionDelegate::close()
     // set the entity to 0
     this->ddsc_entity = DDS_HANDLE_NIL;
     // but don't close the entity as it is the parent entity (fix this above as per above todo)
-    // ConditionDelegate::close(ddsc_entity);
+    // ConditionDelegate::detach_and_close(ddsc_entity);
 }
 
 void

diff --git a/src/ddscxx/src/org/eclipse/cyclonedds/sub/cond/ReadConditionDelegate.cpp b/src/ddscxx/src/org/eclipse/cyclonedds/sub/cond/ReadConditionDelegate.cpp
@@ -75,7 +75,7 @@ org::eclipse::cyclonedds::sub::cond::ReadConditionDelegate::close()
     this->check();
     org::eclipse::cyclonedds::core::ScopedObjectLock scopedLock(*this);
     QueryDelegate::deinit();
-    ConditionDelegate::close(ddsc_entity);
+    ConditionDelegate::detach_and_close(ddsc_entity);
 }
 
 bool