From 81edd7e07a1fae2412c3e42e74a31b992aa72669 Mon Sep 17 00:00:00 2001 From: Simon Cooper Date: Thu, 5 Jan 2017 14:08:17 +0000 Subject: [PATCH 1/3] CURATOR-375 - fix thread interruption being reported using thread interrupted flag and an exception at the same time --- .../src/main/java/org/apache/curator/ConnectionState.java | 1 - .../main/java/org/apache/curator/utils/ThreadUtils.java | 4 +++- .../apache/curator/framework/imps/CreateBuilderImpl.java | 1 - .../apache/curator/framework/imps/DeleteBuilderImpl.java | 1 - .../curator/framework/recipes/nodes/PersistentNode.java | 7 ++++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/curator-client/src/main/java/org/apache/curator/ConnectionState.java b/curator-client/src/main/java/org/apache/curator/ConnectionState.java index 7044ddf884..c41b4cf0c2 100644 --- a/curator-client/src/main/java/org/apache/curator/ConnectionState.java +++ b/curator-client/src/main/java/org/apache/curator/ConnectionState.java @@ -117,7 +117,6 @@ public void close() throws IOException } catch ( Exception e ) { - ThreadUtils.checkInterrupted(e); throw new IOException(e); } finally diff --git a/curator-client/src/main/java/org/apache/curator/utils/ThreadUtils.java b/curator-client/src/main/java/org/apache/curator/utils/ThreadUtils.java index 74b4e4008c..bc936047ec 100644 --- a/curator-client/src/main/java/org/apache/curator/utils/ThreadUtils.java +++ b/curator-client/src/main/java/org/apache/curator/utils/ThreadUtils.java @@ -31,12 +31,14 @@ public class ThreadUtils { private static final Logger log = LoggerFactory.getLogger(ThreadUtils.class); - public static void checkInterrupted(Throwable e) + public static boolean checkInterrupted(Throwable e) { if ( e instanceof InterruptedException ) { Thread.currentThread().interrupt(); + return true; } + return false; } public static ExecutorService newSingleThreadExecutor(String processName) diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java index 98c2a05dd9..5aa012ecaf 100644 --- a/curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java +++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java @@ -485,7 +485,6 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw } catch ( Exception e) { - ThreadUtils.checkInterrupted(e); if ( ( e instanceof KeeperException.ConnectionLossException || !( e instanceof KeeperException )) && protectedId != null ) { diff --git a/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java b/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java index 7fbe36e08a..adad0ff823 100644 --- a/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java +++ b/curator-framework/src/main/java/org/apache/curator/framework/imps/DeleteBuilderImpl.java @@ -267,7 +267,6 @@ public Void call() throws Exception } catch ( Exception e ) { - ThreadUtils.checkInterrupted(e); //Only retry a guaranteed delete if it's a retryable error if( (RetryLoop.isRetryException(e) || (e instanceof InterruptedException)) && guaranteed ) { diff --git a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java index 5f94ea197b..5d8a2e4c98 100644 --- a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java +++ b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java @@ -293,8 +293,10 @@ public void close() throws IOException } catch ( Exception e ) { - ThreadUtils.checkInterrupted(e); - throw new IOException(e); + if ( !ThreadUtils.checkInterrupted(e) ) + { + throw new IOException(e); + } } } @@ -369,7 +371,6 @@ private void createNode() } catch ( Exception e ) { - ThreadUtils.checkInterrupted(e); throw new RuntimeException("Creating node. BasePath: " + basePath, e); // should never happen unless there's a programming error - so throw RuntimeException } } From 27b7509ef30f2f875b39ff0e227416f83b960185 Mon Sep 17 00:00:00 2001 From: Simon Cooper Date: Tue, 10 Jan 2017 16:58:11 +0000 Subject: [PATCH 2/3] add test case for PersistentNode --- .../recipes/nodes/TestPersistentNode.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java index ac8c65d304..5ecdbe4236 100644 --- a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java +++ b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java @@ -27,6 +27,8 @@ import org.apache.zookeeper.CreateMode; import org.testng.Assert; import org.testng.annotations.Test; + +import java.io.IOException; import java.util.concurrent.TimeUnit; public class TestPersistentNode extends BaseClassForTests @@ -136,4 +138,39 @@ public void testQuickCloseNodeExists() throws Exception CloseableUtils.closeQuietly(client); } } + + @Test + public void testInterruption() throws Exception + { + Timing timing = new Timing(); + PersistentNode pen = null; + CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), timing.session(), timing.connection(), new RetryOneTime(1)); + try + { + client.start(); + client.create().creatingParentsIfNeeded().forPath("/test"); + + pen = new PersistentNode(client, CreateMode.PERSISTENT, false, "/test", new byte[0]); + pen.start(); + + // interrupt once + Thread.currentThread().interrupt(); + + try { + pen.close(); + } + catch (IOException expected) { + if (!(expected.getCause() instanceof InterruptedException)) { + throw expected; + } + } + + timing.sleepABit(); + } + finally + { + CloseableUtils.closeQuietly(pen); + CloseableUtils.closeQuietly(client); + } + } } From dff9bf182763c08aabc7dbeb646a31cfad94cd33 Mon Sep 17 00:00:00 2001 From: Simon Cooper Date: Tue, 10 Jan 2017 17:03:43 +0000 Subject: [PATCH 3/3] make test explicitly fail if interrupt exposed more than once --- .../recipes/nodes/TestPersistentNode.java | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java index 5ecdbe4236..7e767682f5 100644 --- a/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java +++ b/curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentNode.java @@ -152,20 +152,33 @@ public void testInterruption() throws Exception pen = new PersistentNode(client, CreateMode.PERSISTENT, false, "/test", new byte[0]); pen.start(); + Assert.assertTrue(pen.waitForInitialCreate(timing.milliseconds(), TimeUnit.MILLISECONDS)); // interrupt once Thread.currentThread().interrupt(); + int interruptCount = 0; + try { pen.close(); } catch (IOException expected) { - if (!(expected.getCause() instanceof InterruptedException)) { + if (expected.getCause() instanceof InterruptedException) { + interruptCount++; + } + else { throw expected; } } - timing.sleepABit(); + try { + timing.sleepABit(); + } + catch (InterruptedException expected) { + interruptCount++; + } + + Assert.assertEquals(interruptCount, 1); } finally {