You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by kl...@apache.org on 2019/08/02 22:12:02 UTC
[geode] 02/02: GEODE-7001: Prevent potential NPE in LocalRegion
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 <al...@pivotal.io>
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 <al...@pivotal.io>
Co-authored-by: Kirk Lund <kl...@apache.org>
---
.../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();
+ }
+}