pivotal-jbarrett commented on a change in pull request #597:
URL: https://github.com/apache/geode-native/pull/597#discussion_r422321681
##########
File path: cppcache/src/CacheImpl.cpp
##########
@@ -640,13 +640,16 @@ void CacheImpl::readyForEvents() {
}
}
-bool CacheImpl::isPoolInMultiuserMode(std::shared_ptr<Region> regionPtr) {
- const auto& poolName = regionPtr->getAttributes().getPoolName();
+bool CacheImpl::isPoolInMultiuserMode(std::shared_ptr<Region> region) {
+ const auto& poolName = region->getAttributes().getPoolName();
if (!poolName.empty()) {
- auto poolPtr = regionPtr->getCache().getPoolManager().find(poolName);
- if (poolPtr != nullptr && !poolPtr->isDestroyed()) {
- return poolPtr->getMultiuserAuthentication();
+ auto pool = static_cast<RegionInternal&>(*region)
+ .getCacheImpl()
+ ->getPoolManager()
+ .find(poolName);
+ if (pool && !pool->isDestroyed()) {
+ return pool->getMultiuserAuthentication();
Review comment:
It's a really good question. I suppose it could use `this` and some that
the `region` is owned by `this` `CacheImpl`. Even better refactor.
Testing this would require a concurrency test framework we don't have and
then magical timing. You could write a flaky test that tries over and over in
hopes of hitting it but not hitting it doesn't prove it is fixed. Through
analysis of the object lifecycle we can at least guarantee that if we get into
this call that `this` `CacheImpl` has not been destructed.
----------------------------------------------------------------
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:
us...@infra.apache.org
