You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/20 14:16:12 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #772: GEODE-9056: Replace ACE_Semaphore
gaussianrecurrence opened a new pull request #772:
URL: https://github.com/apache/geode-native/pull/772
- In order to progress in the task of getting rid of ACE library, a
replacement is needed for ACE_Semaphore.
- Currently there is no suitable Boost alternative and C++
implementation is not in the standard until C++20. Hence STL like
implementation was added as binary_semaphore.
- Replaced all instances of ACE_Semaphore by binary_semaphore.
- Fixed ill semaphore logic in several cases in which running token was
checked twice without any specific need.
--
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
[GitHub] [geode-native] pdxcodemonkey merged pull request #772: GEODE-9056: Replace ACE_Semaphore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #772:
URL: https://github.com/apache/geode-native/pull/772
--
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
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #772: GEODE-9056: Replace ACE_Semaphore
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #772:
URL: https://github.com/apache/geode-native/pull/772#discussion_r600704330
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -72,7 +72,7 @@ void TcrConnectionManager::init(bool isPool) {
}
auto &props = m_cache->getDistributedSystem().getSystemProperties();
m_isDurable = !props.durableClientId().empty();
- auto pingInterval = (props.pingInterval() / 2);
+ auto pingInterval = props.pingInterval();
Review comment:
This is something interesting hehe. Thing is previous implementation called twice to semaphore acquire. The implementation was clearly wrong. But in order to solve the issue the ping interval was divided by 2, et voilà.
So now the right interval is used and ping semaphore acquire is just called once.
--
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
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #772: GEODE-9056: Replace ACE_Semaphore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #772:
URL: https://github.com/apache/geode-native/pull/772#discussion_r600702071
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -276,22 +276,21 @@ int TcrConnectionManager::checkConnection(const ACE_Time_Value &,
int TcrConnectionManager::checkRedundancy(const ACE_Time_Value &,
const void *) {
- m_redundancySema.release();
+ redundancy_semaphore_.release();
return 0;
}
void TcrConnectionManager::failover(std::atomic<bool> &isRunning) {
LOGFINE("TcrConnectionManager: starting failover thread");
+
+ failover_semaphore_.acquire();
while (isRunning) {
- m_failoverSema.acquire();
- if (isRunning && !m_isNetDown) {
+ if (!m_isNetDown) {
try {
std::lock_guard<decltype(m_distMngrsLock)> guard(m_distMngrsLock);
for (const auto &it : m_distMngrs) {
it->failover();
}
Review comment:
Yep, the `failover()` call is supposed to do a `release()`. Ugh.
--
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
[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #772: GEODE-9056: Replace ACE_Semaphore
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #772:
URL: https://github.com/apache/geode-native/pull/772#discussion_r600655086
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -72,7 +72,7 @@ void TcrConnectionManager::init(bool isPool) {
}
auto &props = m_cache->getDistributedSystem().getSystemProperties();
m_isDurable = !props.durableClientId().empty();
- auto pingInterval = (props.pingInterval() / 2);
+ auto pingInterval = props.pingInterval();
Review comment:
Any idea why this interval was originally divided by 2?
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -302,7 +301,10 @@ void TcrConnectionManager::failover(std::atomic<bool> &isRunning) {
"different endpoint");
}
}
+
+ failover_semaphore_.acquire();
Review comment:
Okay, so here's the other acquire call previously in the middle of the while loop. So again I wonder why we're attempting to acquire this semaphore that is already held as of line 286 above. What's going on here?
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -276,22 +276,21 @@ int TcrConnectionManager::checkConnection(const ACE_Time_Value &,
int TcrConnectionManager::checkRedundancy(const ACE_Time_Value &,
const void *) {
- m_redundancySema.release();
+ redundancy_semaphore_.release();
return 0;
}
void TcrConnectionManager::failover(std::atomic<bool> &isRunning) {
LOGFINE("TcrConnectionManager: starting failover thread");
+
+ failover_semaphore_.acquire();
while (isRunning) {
- m_failoverSema.acquire();
- if (isRunning && !m_isNetDown) {
+ if (!m_isNetDown) {
try {
std::lock_guard<decltype(m_distMngrsLock)> guard(m_distMngrsLock);
for (const auto &it : m_distMngrs) {
it->failover();
}
Review comment:
Wow, some crazy logic in the original version here. Do you suppose they thought it was possible to "lose" the already acquired semaphore in the call to `it->failover()`? I don't understand the reasoning behind attempting to acquire the semaphore that's clearly already held.
##########
File path: cppcache/src/TcrConnectionManager.cpp
##########
@@ -302,7 +301,10 @@ void TcrConnectionManager::failover(std::atomic<bool> &isRunning) {
"different endpoint");
}
}
+
+ failover_semaphore_.acquire();
Review comment:
All right, so these are "counting" semaphores, with a max of 1, I see what's going on. Still weird, but okay.
--
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