You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by ke...@apache.org on 2023/08/13 02:34:11 UTC
[curator] branch master updated: CURATOR-626: Postpone null namespace facade creation to avoid partial initialization (#475)
This is an automated email from the ASF dual-hosted git repository.
kezhuw pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/curator.git
The following commit(s) were added to refs/heads/master by this push:
new 97fe626c CURATOR-626: Postpone null namespace facade creation to avoid partial initialization (#475)
97fe626c is described below
commit 97fe626c25d57d47816f95486be77ef39646ed59
Author: Kezhu Wang <ke...@apache.org>
AuthorDate: Sun Aug 13 10:34:06 2023 +0800
CURATOR-626: Postpone null namespace facade creation to avoid partial initialization (#475)
In construction of `CuratorFrameworkImpl`, it constructs a sub class
`NamespaceFacade` for commonly used `nullNamespace`. Due to this
recursive construction, `nullNamespace` has uninitialized fields which
cause NPT in certain cases.
This commit solves this by postponing `nullNamespace` creation.
---
curator-framework/pom.xml | 8 ++++-
.../framework/imps/CuratorFrameworkImpl.java | 4 +--
.../framework/imps/NamespaceFacadeCache.java | 22 ++++++++++++--
.../curator/framework/imps/TestFramework.java | 35 ++++++++++++++++++++++
curator-test-zk35/pom.xml | 12 ++++++++
curator-test-zk36/pom.xml | 6 ++++
pom.xml | 6 ++++
7 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/curator-framework/pom.xml b/curator-framework/pom.xml
index a8817179..69b8d1d3 100644
--- a/curator-framework/pom.xml
+++ b/curator-framework/pom.xml
@@ -100,7 +100,13 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
-
+
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
index 9de2e632..019897b3 100644
--- a/curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
+++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
@@ -283,14 +283,14 @@ public class CuratorFrameworkImpl implements CuratorFramework {
compressionProvider = parent.compressionProvider;
aclProvider = parent.aclProvider;
namespaceFacadeCache = parent.namespaceFacadeCache;
- namespace = new NamespaceImpl(this, null);
+ namespace = parent.namespace;
state = parent.state;
authInfos = parent.authInfos;
useContainerParentsIfAvailable = parent.useContainerParentsIfAvailable;
connectionStateErrorPolicy = parent.connectionStateErrorPolicy;
internalConnectionHandler = parent.internalConnectionHandler;
schemaSet = parent.schemaSet;
- ensembleTracker = null;
+ ensembleTracker = parent.ensembleTracker;
runSafeService = parent.runSafeService;
}
diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceFacadeCache.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceFacadeCache.java
index 80b07149..a17865db 100644
--- a/curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceFacadeCache.java
+++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/NamespaceFacadeCache.java
@@ -24,10 +24,11 @@ import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
class NamespaceFacadeCache {
private final CuratorFrameworkImpl client;
- private final NamespaceFacade nullNamespace;
+ private final AtomicReference<NamespaceFacade> nullNamespace;
private final CacheLoader<String, NamespaceFacade> loader = new CacheLoader<String, NamespaceFacade>() {
@Override
public NamespaceFacade load(String namespace) throws Exception {
@@ -40,12 +41,27 @@ class NamespaceFacadeCache {
NamespaceFacadeCache(CuratorFrameworkImpl client) {
this.client = client;
- nullNamespace = new NamespaceFacade(client, null);
+ this.nullNamespace = new AtomicReference<>(null);
+ }
+
+ private NamespaceFacade getNullNamespace() {
+ NamespaceFacade facade = nullNamespace.get();
+ if (facade != null) {
+ return facade;
+ }
+ facade = new NamespaceFacade(client, null);
+ if (!nullNamespace.compareAndSet(null, facade)) {
+ facade = nullNamespace.get();
+ }
+ return facade;
}
NamespaceFacade get(String namespace) {
try {
- return (namespace != null) ? cache.get(namespace) : nullNamespace;
+ if (namespace == null) {
+ return getNullNamespace();
+ }
+ return cache.get(namespace);
} catch (ExecutionException e) {
throw new RuntimeException(e); // should never happen
}
diff --git a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFramework.java b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFramework.java
index 63adc76c..ea7fc048 100644
--- a/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFramework.java
+++ b/curator-framework/src/test/java/org/apache/curator/framework/imps/TestFramework.java
@@ -30,6 +30,7 @@ import static org.junit.jupiter.api.Assertions.fail;
import com.google.common.collect.Lists;
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
@@ -82,6 +83,8 @@ import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
+import org.mockito.MockedConstruction;
+import org.mockito.Mockito;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -731,6 +734,38 @@ public class TestFramework extends BaseClassForTests {
}
}
+ @Test
+ public void testNullNamespace() {
+ CuratorFramework client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .ensembleTracker(true)
+ .retryPolicy(new RetryOneTime(1))
+ .build();
+
+ client.start();
+
+ CuratorFrameworkImpl nullNamespace = (CuratorFrameworkImpl) client.usingNamespace(null);
+
+ assertNotNull(nullNamespace.getEnsembleTracker());
+ assertNotNull(nullNamespace.getNamespaceFacadeCache());
+ nullNamespace.runSafe(() -> {}).join();
+
+ CloseableUtils.closeQuietly(client);
+ }
+
+ @Test
+ public void testNoPartialConstruction() {
+ try (MockedConstruction<CuratorFrameworkImpl> construction =
+ Mockito.mockConstruction(CuratorFrameworkImpl.class)) {
+ CuratorFrameworkFactory.Builder builder = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .retryPolicy(new RetryOneTime(1));
+ // Invoke constructor directly to make the construction more visible.
+ CuratorFramework client = new CuratorFrameworkImpl(builder);
+ assertEquals(Collections.singletonList(client), construction.constructed());
+ }
+ }
+
@Test
public void testCustomCallback() throws Exception {
CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1));
diff --git a/curator-test-zk35/pom.xml b/curator-test-zk35/pom.xml
index 84cf0df0..9761dd28 100644
--- a/curator-test-zk35/pom.xml
+++ b/curator-test-zk35/pom.xml
@@ -171,6 +171,18 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
+ <scope>test</scope>
+ </dependency>
+
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
diff --git a/curator-test-zk36/pom.xml b/curator-test-zk36/pom.xml
index 93774a2a..84f655e5 100644
--- a/curator-test-zk36/pom.xml
+++ b/curator-test-zk36/pom.xml
@@ -190,6 +190,12 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <scope>test</scope>
+ </dependency>
+
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
diff --git a/pom.xml b/pom.xml
index c406b913..b9ede533 100644
--- a/pom.xml
+++ b/pom.xml
@@ -387,6 +387,12 @@
<version>${mockito-version}</version>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-inline</artifactId>
+ <version>${mockito-version}</version>
+ </dependency>
+
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>