Repository: curator Updated Branches: refs/heads/CURATOR-141 [created] 0d1397af1
CURATOR-141 Don't make calls to the client after PathChildrenCache has been closed. The test case is a bit weird because it needed to intercept logging. Also, I've added slf4j-log4j12 as a test dependency to control logging. I think it should be added to the other poms as well, otherwise logging seems to get lost. Project: http://git-wip-us.apache.org/repos/asf/curator/repo Commit: http://git-wip-us.apache.org/repos/asf/curator/commit/f2f1953c Tree: http://git-wip-us.apache.org/repos/asf/curator/tree/f2f1953c Diff: http://git-wip-us.apache.org/repos/asf/curator/diff/f2f1953c Branch: refs/heads/CURATOR-141 Commit: f2f1953c544398cb6db53a2ac59b02719cc83fa3 Parents: d2c37d0 Author: Karel Vervaeke <[email protected]> Authored: Thu Aug 14 13:46:03 2014 +0200 Committer: Karel Vervaeke <[email protected]> Committed: Thu Aug 14 13:46:03 2014 +0200 ---------------------------------------------------------------------- curator-recipes/pom.xml | 6 ++ .../recipes/cache/PathChildrenCache.java | 4 + .../recipes/cache/TestPathChildrenCache.java | 85 +++++++++++++++++++- pom.xml | 6 ++ 4 files changed, 100 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/curator/blob/f2f1953c/curator-recipes/pom.xml ---------------------------------------------------------------------- diff --git a/curator-recipes/pom.xml b/curator-recipes/pom.xml index 13aa475..37155c9 100644 --- a/curator-recipes/pom.xml +++ b/curator-recipes/pom.xml @@ -61,5 +61,11 @@ <artifactId>mockito-core</artifactId> <scope>test</scope> </dependency> + + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-log4j12</artifactId> + <scope>test</scope> + </dependency> </dependencies> </project> http://git-wip-us.apache.org/repos/asf/curator/blob/f2f1953c/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java ---------------------------------------------------------------------- diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java index d555508..57c6e92 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java @@ -487,6 +487,10 @@ public class PathChildrenCache implements Closeable @Override public void processResult(CuratorFramework client, CuratorEvent event) throws Exception { + if (PathChildrenCache.this.state.get().equals(State.CLOSED)) { + // This ship is closed, don't handle the callback + return; + } if ( event.getResultCode() == KeeperException.Code.OK.intValue() ) { processChildren(event.getChildren(), mode); http://git-wip-us.apache.org/repos/asf/curator/blob/f2f1953c/curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java ---------------------------------------------------------------------- diff --git a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java index 60f2e88..70156ff 100644 --- a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java +++ b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestPathChildrenCache.java @@ -18,22 +18,44 @@ */ package org.apache.curator.framework.recipes.cache; +import com.google.common.base.Function; +import com.google.common.base.Joiner; +import com.google.common.collect.Collections2; import com.google.common.collect.Lists; import org.apache.curator.framework.CuratorFramework; import org.apache.curator.framework.CuratorFrameworkFactory; import org.apache.curator.framework.api.UnhandledErrorListener; +import org.apache.curator.framework.imps.CuratorFrameworkImpl; import org.apache.curator.retry.RetryOneTime; import org.apache.curator.test.BaseClassForTests; import org.apache.curator.test.KillSession; import org.apache.curator.test.Timing; import org.apache.curator.utils.CloseableUtils; +import org.apache.log4j.Appender; +import org.apache.log4j.AppenderSkeleton; +import org.apache.log4j.Level; +import org.apache.log4j.Logger; +import org.apache.log4j.SimpleLayout; +import org.apache.log4j.spi.LoggingEvent; import org.apache.zookeeper.CreateMode; import org.apache.zookeeper.KeeperException; import org.testng.Assert; import org.testng.annotations.Test; + import java.util.Collection; import java.util.List; -import java.util.concurrent.*; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Exchanger; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -76,6 +98,67 @@ public class TestPathChildrenCache extends BaseClassForTests } @Test + public void testClientClosedDuringRefreshErrorMessage() throws Exception + { + Timing timing = new Timing(); + + // Fiddle with logging so we can intercept the error events for org.apache.curator + final List<LoggingEvent> events = Lists.newArrayList(); + Collection<String> messages = Collections2.transform(events, new Function<LoggingEvent, String>() { + @Override + public String apply(LoggingEvent loggingEvent) { + return loggingEvent.getRenderedMessage(); + } + }); + Appender appender = new AppenderSkeleton(true) { + @Override + protected void append(LoggingEvent event) { + if (event.getLevel().equals(Level.ERROR)) { + events.add(event); + } + } + + @Override + public void close() { + + } + + @Override + public boolean requiresLayout() { + return false; + } + }; + appender.setLayout(new SimpleLayout()); + Logger rootLogger = Logger.getLogger("org.apache.curator"); + rootLogger.addAppender(appender); + + // Check that the logging setup didn't change in a way that breaks this test + Logger.getLogger(CuratorFrameworkImpl.class).error("Just checking"); + Assert.assertTrue(messages.contains("Just checking"), + "The test error message was not logged, this test may be broken"); + + // try to reproduce a bunch of times because it doesn't happen reliably + for (int i = 0; i < 50; i++) { + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1)); + client.start(); + try { + PathChildrenCache cache = new PathChildrenCache(client, "/test", true); + cache.start(PathChildrenCache.StartMode.BUILD_INITIAL_CACHE); + client.newNamespaceAwareEnsurePath("/test/aaa").ensure(client.getZookeeperClient()); + client.setData().forPath("/test/aaa", new byte[]{1, 2, 3, 4, 5}); + cache.rebuildNode("/test/aaa"); + CloseableUtils.closeQuietly(cache); + } finally { + CloseableUtils.closeQuietly(client); + } + } + + Assert.assertEquals(messages.size(), 1, "There should not be any error events except for the test message, " + + "but got:\n" + Joiner.on("\n").join(messages)); + + } + + @Test public void testAsyncInitialPopulation() throws Exception { Timing timing = new Timing(); http://git-wip-us.apache.org/repos/asf/curator/blob/f2f1953c/pom.xml ---------------------------------------------------------------------- diff --git a/pom.xml b/pom.xml index 2129e56..635819b 100644 --- a/pom.xml +++ b/pom.xml @@ -279,6 +279,12 @@ </dependency> <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-log4j12</artifactId> + <version>1.7.6</version> + </dependency> + + <dependency> <groupId>org.mockito</groupId> <artifactId>mockito-core</artifactId> <version>1.9.5</version>
