This is an automated email from the ASF dual-hosted git repository. klund pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
commit c2ef3c682c885b4d49aad51edc1c11e80553bdfa Author: Aaron Lindsey <[email protected]> AuthorDate: Fri Aug 2 11:18:01 2019 -0700 GEODE-7001: Prevent potential NPE in LocalRegion Prevent potential NPE during LocalRegion construction when stat sampling is enabled. Specifically, if getLocalSize() is invoked by the callback sampler before the RegionMap inside LocalRegion is created then it will throw an NPE. Co-authored-by: Aaron Lindsey <[email protected]> Co-authored-by: Kirk Lund <[email protected]> --- .../apache/geode/internal/cache/LocalRegion.java | 24 +++--- .../internal/cache/AbstractRegionJUnitTest.java | 6 +- .../geode/internal/cache/LocalRegionTest.java | 92 ++++++++++++++++++++++ 3 files changed, 110 insertions(+), 12 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java index 6f3a41d..49ca506 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java @@ -540,7 +540,11 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory, InternalDataView internalDataView) throws DiskAccessException { this(regionName, attrs, parentRegion, cache, internalRegionArgs, internalDataView, RegionMapFactory::createVM, new DefaultServerRegionProxyConstructor(), - new DefaultEntryEventFactory(), poolName -> (PoolImpl) PoolManager.find(poolName)); + new DefaultEntryEventFactory(), poolName -> (PoolImpl) PoolManager.find(poolName), + (LocalRegion region) -> new RegionPerfStats( + cache.getInternalDistributedSystem().getStatisticsManager(), + "RegionStats-" + regionName, cache.getCachePerfStats(), + region, cache.getMeterRegistry())); } @VisibleForTesting @@ -548,7 +552,8 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory, InternalCache cache, InternalRegionArguments internalRegionArgs, InternalDataView internalDataView, RegionMapConstructor regionMapConstructor, ServerRegionProxyConstructor serverRegionProxyConstructor, - EntryEventFactory entryEventFactory, PoolFinder poolFinder) + EntryEventFactory entryEventFactory, PoolFinder poolFinder, + java.util.function.Function<LocalRegion, RegionPerfStats> regionPerfStatsFactory) throws DiskAccessException { super(cache, attrs, regionName, internalRegionArgs, poolFinder); @@ -587,6 +592,11 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory, } initializingRegion.set(this); + diskStoreImpl = findDiskStore(attrs, internalRegionArgs); + diskRegion = createDiskRegion(internalRegionArgs); + entries = createRegionMap(internalRegionArgs); + subregions = new ConcurrentHashMap(); + if (internalRegionArgs.getCachePerfStatsHolder() != null) { hasOwnStats = false; cachePerfStats = internalRegionArgs.getCachePerfStatsHolder().getCachePerfStats(); @@ -597,18 +607,10 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory, cachePerfStats = cache.getCachePerfStats(); } else { hasOwnStats = true; - cachePerfStats = - new RegionPerfStats(cache.getInternalDistributedSystem().getStatisticsManager(), - "RegionStats-" + regionName, cache.getCachePerfStats(), - this, cache.getMeterRegistry()); + cachePerfStats = regionPerfStatsFactory.apply(this); } } - diskStoreImpl = findDiskStore(attrs, internalRegionArgs); - diskRegion = createDiskRegion(internalRegionArgs); - entries = createRegionMap(internalRegionArgs); - subregions = new ConcurrentHashMap(); - // we only need a destroy lock if this is a root if (parentRegion == null) { initRoot(); diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java index a04f5b7..09c7c5f 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionJUnitTest.java @@ -20,6 +20,7 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import java.util.Set; +import java.util.function.Function; import org.junit.Test; @@ -113,8 +114,11 @@ public class AbstractRegionJUnitTest { DiskWriteAttributes diskWriteAttributes = mock(DiskWriteAttributes.class); when(regionAttributes.getDiskWriteAttributes()).thenReturn(diskWriteAttributes); RegionMapConstructor regionMapConstructor = mock(RegionMapConstructor.class); + Function<LocalRegion, RegionPerfStats> regionPerfStatsFactory = + (localRegion) -> mock(RegionPerfStats.class); AbstractRegion region = new LocalRegion("regionName", regionAttributes, null, Fakes.cache(), - new InternalRegionArguments(), null, regionMapConstructor, null, null, null); + new InternalRegionArguments(), null, regionMapConstructor, null, null, null, + regionPerfStatsFactory); return region; } } diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java new file mode 100644 index 0000000..22adf2b --- /dev/null +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more contributor license + * agreements. See the NOTICE file distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. You may obtain a + * copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.geode.internal.cache; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.function.Function; + +import org.junit.Before; +import org.junit.Test; + +import org.apache.geode.cache.DataPolicy; +import org.apache.geode.cache.DiskWriteAttributes; +import org.apache.geode.cache.EvictionAction; +import org.apache.geode.cache.EvictionAttributes; +import org.apache.geode.cache.ExpirationAttributes; +import org.apache.geode.cache.RegionAttributes; +import org.apache.geode.distributed.internal.DSClock; +import org.apache.geode.distributed.internal.InternalDistributedSystem; +import org.apache.geode.internal.cache.AbstractRegion.PoolFinder; +import org.apache.geode.internal.cache.LocalRegion.RegionMapConstructor; +import org.apache.geode.internal.cache.LocalRegion.ServerRegionProxyConstructor; + +public class LocalRegionTest { + + private RegionAttributes regionAttributes; + private InternalCache cache; + private InternalRegionArguments internalRegionArguments; + private InternalDataView internalDataView; + private RegionMapConstructor regionMapConstructor; + private ServerRegionProxyConstructor serverRegionProxyConstructor; + private EntryEventFactory entryEventFactory; + private PoolFinder poolFinder; + + @Before + public void setup() { + cache = mock(InternalCache.class); + entryEventFactory = mock(EntryEventFactory.class); + internalDataView = mock(InternalDataView.class); + internalRegionArguments = mock(InternalRegionArguments.class); + poolFinder = mock(PoolFinder.class); + regionAttributes = mock(RegionAttributes.class); + regionMapConstructor = mock(RegionMapConstructor.class); + serverRegionProxyConstructor = mock(ServerRegionProxyConstructor.class); + + DiskWriteAttributes diskWriteAttributes = mock(DiskWriteAttributes.class); + EvictionAttributes evictionAttributes = mock(EvictionAttributes.class); + ExpirationAttributes expirationAttributes = mock(ExpirationAttributes.class); + InternalDistributedSystem internalDistributedSystem = mock(InternalDistributedSystem.class); + + when(cache.getInternalDistributedSystem()).thenReturn(internalDistributedSystem); + when(evictionAttributes.getAction()).thenReturn(EvictionAction.NONE); + when(internalDistributedSystem.getClock()).thenReturn(mock(DSClock.class)); + when(regionAttributes.getDataPolicy()).thenReturn(DataPolicy.DEFAULT); + when(regionAttributes.getDiskWriteAttributes()).thenReturn(diskWriteAttributes); + when(regionAttributes.getEntryIdleTimeout()).thenReturn(expirationAttributes); + when(regionAttributes.getEntryTimeToLive()).thenReturn(expirationAttributes); + when(regionAttributes.getEvictionAttributes()).thenReturn(evictionAttributes); + when(regionAttributes.getRegionIdleTimeout()).thenReturn(expirationAttributes); + when(regionAttributes.getRegionTimeToLive()).thenReturn(expirationAttributes); + when(regionMapConstructor.create(any(), any(), any())).thenReturn(mock(RegionMap.class)); + } + + @Test + public void getLocalSizeDoesNotThrowNullPointerExceptionDuringConstruction() { + Function<LocalRegion, RegionPerfStats> regionPerfStatsFactory = (localRegion) -> { + localRegion.getLocalSize(); + return mock(RegionPerfStats.class); + }; + + assertThatCode(() -> new LocalRegion("region", regionAttributes, null, cache, + internalRegionArguments, internalDataView, regionMapConstructor, + serverRegionProxyConstructor, entryEventFactory, poolFinder, regionPerfStatsFactory)) + .doesNotThrowAnyException(); + } +}
