[jira] [Commented] (GEODE-9078) Remove ACE mutexes

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-9078?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17335776#comment-17335776 ] 

ASF GitHub Bot commented on GEODE-9078:
---------------------------------------

pivotal-jbarrett commented on a change in pull request #776:
URL: https://github.com/apache/geode-native/pull/776#discussion_r623284694



##########
File path: cppcache/src/AdminRegion.hpp
##########
@@ -48,7 +48,7 @@ class AdminRegion : public std::enable_shared_from_this<AdminRegion> {
   ThinClientBaseDM* m_distMngr;
   std::string m_fullPath;
   TcrConnectionManager* m_connectionMgr;
-  ACE_RW_Thread_Mutex m_rwLock;
+  boost::shared_mutex mutex_;

Review comment:
       Please match the existing member variable format or update them all to the current standard.

##########
File path: cppcache/src/RemoteQuery.cpp
##########
@@ -125,7 +125,7 @@ GfErrType RemoteQuery::executeNoThrow(
     ThinClientBaseDM* tcdm, std::shared_ptr<CacheableVector> paramList) {
   LOGFINEST("%s: executing query: %s", func, m_queryString.c_str());
 
-  TryReadGuard guard(m_queryService->getLock(), m_queryService->invalid());
+  TryReadGuard guard(m_queryService->getMutex(), m_queryService->invalid());

Review comment:
       This always felt very wrong to me. It's not just that locks across components is a bad smell but that we leave the leak the locking logic too. What if there was an `tryAcquireReadLock` type method that returned a guard object?

##########
File path: cppcache/src/DataOutputInternal.hpp
##########
@@ -23,8 +23,6 @@
 #include <geode/DataOutput.hpp>
 #include <geode/PoolManager.hpp>  // TODO remove
 
-#include "CacheImpl.hpp"  // TODO remove

Review comment:
       Yes!

##########
File path: cppcache/src/ReadWriteLock.cpp
##########
@@ -23,28 +23,28 @@ namespace apache {
 namespace geode {
 namespace client {
 
-TryReadGuard::TryReadGuard(ACE_RW_Thread_Mutex& lock,
-                           const volatile bool& exitCondition)
-    : lock_(lock), isAcquired_(false) {
+TryReadGuard::TryReadGuard(boost::shared_mutex& mutex,
+                           const volatile bool& exit_cond)
+    : mutex_{mutex}, locked_{false} {
   do {
-    if (lock_.tryacquire_read() != -1) {
-      isAcquired_ = true;
+    if (mutex_.try_lock_shared()) {
+      locked_ = true;
       break;
     }
     std::this_thread::yield();
-  } while (!exitCondition);
+  } while (!exit_cond);
 }
 
-TryWriteGuard::TryWriteGuard(ACE_RW_Thread_Mutex& lock,
-                             const volatile bool& exitCondition)
-    : lock_(lock), isAcquired_(false) {
+TryWriteGuard::TryWriteGuard(boost::shared_mutex& mutex,
+                             const volatile bool& exit_cond)
+    : mutex_{mutex}, locked_{false} {

Review comment:
       What do you think about replacing this hot spin try lock with a condition variable that is signaled. Almost all these are around some shutdown flag, which doesn't change frequently so if the lock is contended we are just eating CPU for nothing.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> Remove ACE mutexes
> ------------------
>
>                 Key: GEODE-9078
>                 URL: https://issues.apache.org/jira/browse/GEODE-9078
>             Project: Geode
>          Issue Type: Task
>          Components: native client
>            Reporter: Mario Salazar de Torres
>            Assignee: Mario Salazar de Torres
>            Priority: Major
>              Labels: obliterate-ace, pull-request-available
>
> *AS AN* geode-native contributor
>  *I WANT TO* remove all occurrences of ACE mutexes
>  *SO THAT* we can get rid of ACE for good



--
This message was sent by Atlassian Jira
(v8.3.4#803005)