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:
[email protected]
