----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51385/#review146705 -----------------------------------------------------------
While it's okay to synchronize like that in checkExistingCache() I think it might be more appropriate to synchronize on CacheFactory.class in basicCreate() and fold the checkExistingClass() method into that method since that's the only place it's used. I'm concerned that the static "instance" variable is being set in GemFireCacheImpl.initialize() outside of synchronization. It used to be that there was only one path to basicCreate() and that method synchronized on CacheFactory.class, but now there appear to be other paths to it, like client-cache creation, that don't synchronize. - Bruce Schuchardt On Aug. 24, 2016, 6:30 p.m., Hitesh Khamesra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51385/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2016, 6:30 p.m.) > > > Review request for geode, Bruce Schuchardt and Darrel Schneider. > > > Repository: geode > > > Description > ------- > > 1. When cache is closed need to return null from "checkExistingCache" > 2. Need to synchronize cache creation > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/internal/cache/GemFireCacheImpl.java > 76a7bad > > Diff: https://reviews.apache.org/r/51385/diff/ > > > Testing > ------- > > > Thanks, > > Hitesh Khamesra > >
