Skip to content

Commit 3f631ac

Browse files
chevarischevaris
authored
Fix PersistentTtlNode not deleted if touch node is never created (#1260)
Closes #1258. See also CURATOR-545, ZOOKEEPER-3546(apache/zookeeper#1138).
1 parent 1bd8450 commit 3f631ac

2 files changed

Lines changed: 83 additions & 32 deletions

File tree

‎curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java‎

Lines changed: 33 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919

2020
package org.apache.curator.framework.recipes.nodes;
2121

22+
import com.google.common.annotations.VisibleForTesting;
2223
import java.io.Closeable;
2324
import java.io.IOException;
2425
import java.util.Objects;
@@ -37,7 +38,7 @@
3738

3839
/**
3940
* <p>
40-
* Manages a {@link PersistentNode} that uses {@link CreateMode#CONTAINER}. Asynchronously
41+
* Manages a {@link PersistentNode} that uses {@link CreateMode#PERSISTENT_WITH_TTL}. Asynchronously
4142
* it creates or updates a child on the persistent node that is marked with a provided TTL.
4243
* </p>
4344
*
@@ -46,7 +47,7 @@
4647
* a method of having the parent node deleted if the TTL expires. i.e. if the process
4748
* that is running the PersistentTtlNode crashes and the TTL elapses, first the child node
4849
* will be deleted due to the TTL expiration and then the parent node will be deleted as it's
49-
* a container node with no children.
50+
* a TTL node with no children.
5051
* </p>
5152
*
5253
* <p>
@@ -159,46 +160,46 @@ public PersistentTtlNode(
159160
this.client = Objects.requireNonNull(client, "client cannot be null");
160161
this.ttlMs = ttlMs;
161162
this.touchScheduleFactor = touchScheduleFactor;
162-
node = new PersistentNode(client, CreateMode.CONTAINER, false, path, initData, useParentCreation) {
163-
@Override
164-
protected void deleteNode() {
165-
// NOP
166-
}
167-
};
163+
node =
164+
new PersistentNode(
165+
client, CreateMode.PERSISTENT_WITH_TTL, false, path, initData, ttlMs, useParentCreation) {
166+
@Override
167+
protected void deleteNode() {
168+
// NOP
169+
}
170+
};
168171
this.executorService = Objects.requireNonNull(executorService, "executorService cannot be null");
169172
childPath = ZKPaths.makePath(Objects.requireNonNull(path, "path cannot be null"), childNodeName);
170173
}
171174

175+
@VisibleForTesting
176+
void touch() {
177+
try {
178+
try {
179+
client.setData().forPath(childPath);
180+
} catch (KeeperException.NoNodeException e) {
181+
client.create()
182+
.orSetData()
183+
.withTtl(ttlMs)
184+
.withMode(CreateMode.PERSISTENT_WITH_TTL)
185+
.forPath(childPath);
186+
}
187+
} catch (KeeperException.NoNodeException ignore) {
188+
// ignore
189+
} catch (Exception e) {
190+
if (!ThreadUtils.checkInterrupted(e)) {
191+
log.debug("Could not touch child node", e);
192+
}
193+
}
194+
}
195+
172196
/**
173197
* You must call start() to initiate the persistent ttl node
174198
*/
175199
public void start() {
176200
node.start();
177-
178-
Runnable touchTask = new Runnable() {
179-
@Override
180-
public void run() {
181-
try {
182-
try {
183-
client.setData().forPath(childPath);
184-
} catch (KeeperException.NoNodeException e) {
185-
client.create()
186-
.orSetData()
187-
.withTtl(ttlMs)
188-
.withMode(CreateMode.PERSISTENT_WITH_TTL)
189-
.forPath(childPath);
190-
}
191-
} catch (KeeperException.NoNodeException ignore) {
192-
// ignore
193-
} catch (Exception e) {
194-
if (!ThreadUtils.checkInterrupted(e)) {
195-
log.debug("Could not touch child node", e);
196-
}
197-
}
198-
}
199-
};
200201
Future<?> future = executorService.scheduleAtFixedRate(
201-
touchTask, ttlMs / touchScheduleFactor, ttlMs / touchScheduleFactor, TimeUnit.MILLISECONDS);
202+
this::touch, ttlMs / touchScheduleFactor, ttlMs / touchScheduleFactor, TimeUnit.MILLISECONDS);
202203
futureRef.set(future);
203204
}
204205

‎curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,22 +22,28 @@
2222
import static org.apache.curator.framework.recipes.cache.PathChildrenCache.StartMode.BUILD_INITIAL_CACHE;
2323
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
2424
import static org.junit.jupiter.api.Assertions.assertEquals;
25+
import static org.junit.jupiter.api.Assertions.assertFalse;
2526
import static org.junit.jupiter.api.Assertions.assertNotNull;
2627
import static org.junit.jupiter.api.Assertions.assertNull;
2728
import static org.junit.jupiter.api.Assertions.assertThrows;
2829
import static org.junit.jupiter.api.Assertions.assertTrue;
30+
import java.util.concurrent.CountDownLatch;
2931
import java.util.concurrent.Semaphore;
3032
import java.util.concurrent.TimeUnit;
33+
import java.util.concurrent.atomic.AtomicBoolean;
3134
import org.apache.curator.framework.CuratorFramework;
3235
import org.apache.curator.framework.CuratorFrameworkFactory;
3336
import org.apache.curator.framework.recipes.cache.PathChildrenCache;
3437
import org.apache.curator.framework.recipes.cache.PathChildrenCacheEvent;
3538
import org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
39+
import org.apache.curator.framework.recipes.watch.PersistentWatcher;
3640
import org.apache.curator.retry.RetryOneTime;
3741
import org.apache.curator.test.Timing;
3842
import org.apache.curator.test.compatibility.CuratorTestBase;
3943
import org.apache.curator.test.compatibility.Timing2;
4044
import org.apache.curator.utils.ZKPaths;
45+
import org.apache.zookeeper.Watcher;
46+
import org.apache.zookeeper.Watcher.Event.EventType;
4147
import org.junit.jupiter.api.AfterEach;
4248
import org.junit.jupiter.api.BeforeAll;
4349
import org.junit.jupiter.api.BeforeEach;
@@ -187,4 +193,48 @@ public void childEvent(CuratorFramework client, PathChildrenCacheEvent event) th
187193
assertNull(client.checkExists().forPath("/test"));
188194
}
189195
}
196+
197+
@Test
198+
public void testTouchNodeNotCreated() throws Exception {
199+
final String mainPath = "/parent/main";
200+
final String touchPath = ZKPaths.makePath(mainPath, PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
201+
final long testTtlMs = 500L;
202+
final CountDownLatch mainCreatedLatch = new CountDownLatch(1);
203+
final CountDownLatch mainDeletedLatch = new CountDownLatch(1);
204+
final AtomicBoolean touchCreated = new AtomicBoolean();
205+
try (CuratorFramework client =
206+
CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1))) {
207+
client.start();
208+
assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
209+
try (PersistentWatcher watcher = new PersistentWatcher(client, mainPath, true)) {
210+
final Watcher listener = event -> {
211+
final String path = event.getPath();
212+
if (mainPath.equals(path)) {
213+
final EventType type = event.getType();
214+
if (EventType.NodeCreated.equals(type)) {
215+
mainCreatedLatch.countDown();
216+
} else if (EventType.NodeDeleted.equals(type)) {
217+
mainDeletedLatch.countDown();
218+
}
219+
} else if (touchPath.equals(path)) {
220+
touchCreated.set(true);
221+
}
222+
};
223+
watcher.getListenable().addListener(listener);
224+
watcher.start();
225+
try (PersistentTtlNode node = new PersistentTtlNode(client, mainPath, testTtlMs, new byte[0]) {
226+
@Override
227+
void touch() {
228+
// NOP
229+
}
230+
}) {
231+
node.start();
232+
assertTrue(mainCreatedLatch.await(1L, TimeUnit.SECONDS));
233+
}
234+
assertNull(client.checkExists().forPath(touchPath));
235+
assertTrue(mainDeletedLatch.await(3L * testTtlMs, TimeUnit.MILLISECONDS));
236+
assertFalse(touchCreated.get()); // Just to control that touch ZNode never created
237+
}
238+
}
239+
}
190240
}

0 commit comments

Comments
 (0)