You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by cammckenzie <gi...@git.apache.org> on 2014/08/11 03:27:37 UTC
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
GitHub user cammckenzie opened a pull request:
https://github.com/apache/curator/pull/35
CURATOR-79 - Modified the 'withProtection' handling, so that any failure
that is not a ConnectionLossException or other KeeperException is
treated the same as a ConnectionLossException. This means that if the
thread gets interrupted, or encounters some sort of other error, during
creation of a protected zNode, it will remove the affected zNode prior
to rethrowing the exception.
Exposed the debug UnhandledExceptionListener on the CuratorFrameworkImpl
to reproduce this issue via a unit test.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator CURATOR-79
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/35.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #35
----
commit 5398b72f00ccc0f2ea995865fcaf92d73c6c5818
Author: Cam McKenzie <ca...@apache.org>
Date: 2014-08-10T22:13:12Z
CURATOR-79 - Modified the 'withProtection' handling, so that any failure
that is not a ConnectionLossException or other KeeperException is
treated the same as a ConnectionLossException. This means that if the
thread gets interrupted, or encounters some sort of other error, during
creation of a protected zNode, it will remove the affected zNode prior
to rethrowing the exception.
Exposed the debug UnhandledExceptionListener on the CuratorFrameworkImpl
to reproduce this issue via a unit test.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/35
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16053962
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/locks/TestInterProcessMutex.java ---
@@ -107,4 +113,70 @@ public Void call() throws Exception
client.close();
}
}
+
+ /**
+ * See CURATOR-79. If the mutex is interrupted while attempting to acquire a lock it is
+ * possible for the zNode to be created in ZooKeeper, but for Curator to think that it
+ * hasn't been. This causes the next call to acquire() to fail because the an orphaned
+ * zNode has been left behind from the previous call.
+ */
+ @Test
+ public void testInterruptedDuringAcquire() throws Exception
+ {
+ Timing timing = new Timing();
+ final CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1));
+ client.start();
+ final InterProcessMutex lock = new InterProcessMutex(client, LOCK_PATH);
+
+ final AtomicBoolean interruptOnError = new AtomicBoolean(true);
+
+ ((CuratorFrameworkImpl)client).debugUnhandledErrorListener = new UnhandledErrorListener()
+ {
+
+ @Override
+ public void unhandledError(String message, Throwable e)
+ {
+ if(interruptOnError.compareAndSet(true, false))
+ {
+ Thread.currentThread().interrupt();
+ }
+ }
+ };
+
+ //The lock path needs to exist for the deadlock to occur.
+ try {
+ client.create().creatingParentsIfNeeded().forPath(LOCK_PATH);
+ } catch(NodeExistsException e) {
+ }
+
+ try
+ {
+ //Interrupt the current thread. This will cause ensurePath() to fail.
+ //We need to reinterrupt in the debugUnhandledErrorListener above.
+ Thread.currentThread().interrupt();
+ lock.acquire();
+ Assert.fail();
+ }
+ catch(InterruptedException e)
+ {
+ //Expected lock to have failed.
+ Assert.assertTrue(!lock.isOwnedByCurrentThread());
--- End diff --
nit: assertFalse
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16083310
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
--- End diff --
By this I presume you mean catch Exception and then do a conditional instanceof?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on the pull request:
https://github.com/apache/curator/pull/35#issuecomment-52785568
+1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16083843
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
--- End diff --
Yep, agreed, I will put it into a generic catch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16057954
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ if ( protectedId != null )
+ {
+ /*
+ * CURATOR-79 - Handle an runtime exception's here and treat the
+ * same as a connection loss exception. This is necessary as, from
+ * the clients point of view, an exception has been thrown and the
+ * zNode should not exist on ZK. This was causing deadlock in the
+ * locking recipes.
+ */
+ findAndDeleteProtectedNodeInBackground(adjustedPath, protectedId, null);
+ protectedId = UUID.randomUUID().toString();
--- End diff --
Yes, copy the existing ID, generate and assign a new one, and then delete the old one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16053998
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/locks/TestInterProcessMutex.java ---
@@ -107,4 +113,70 @@ public Void call() throws Exception
client.close();
}
}
+
+ /**
+ * See CURATOR-79. If the mutex is interrupted while attempting to acquire a lock it is
+ * possible for the zNode to be created in ZooKeeper, but for Curator to think that it
+ * hasn't been. This causes the next call to acquire() to fail because the an orphaned
+ * zNode has been left behind from the previous call.
+ */
+ @Test
+ public void testInterruptedDuringAcquire() throws Exception
+ {
+ Timing timing = new Timing();
+ final CuratorFramework client = CuratorFrameworkFactory.newClient(server.getConnectString(), new RetryOneTime(1));
+ client.start();
+ final InterProcessMutex lock = new InterProcessMutex(client, LOCK_PATH);
+
+ final AtomicBoolean interruptOnError = new AtomicBoolean(true);
+
+ ((CuratorFrameworkImpl)client).debugUnhandledErrorListener = new UnhandledErrorListener()
+ {
+
+ @Override
+ public void unhandledError(String message, Throwable e)
+ {
+ if(interruptOnError.compareAndSet(true, false))
+ {
+ Thread.currentThread().interrupt();
+ }
+ }
+ };
+
+ //The lock path needs to exist for the deadlock to occur.
+ try {
+ client.create().creatingParentsIfNeeded().forPath(LOCK_PATH);
+ } catch(NodeExistsException e) {
+ }
+
+ try
+ {
+ //Interrupt the current thread. This will cause ensurePath() to fail.
+ //We need to reinterrupt in the debugUnhandledErrorListener above.
+ Thread.currentThread().interrupt();
+ lock.acquire();
+ Assert.fail();
--- End diff --
Can you add a message to the fail(), in case we see a regression in the future?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16055156
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ if ( protectedId != null )
+ {
+ /*
+ * CURATOR-79 - Handle an runtime exception's here and treat the
+ * same as a connection loss exception. This is necessary as, from
+ * the clients point of view, an exception has been thrown and the
+ * zNode should not exist on ZK. This was causing deadlock in the
+ * locking recipes.
+ */
+ findAndDeleteProtectedNodeInBackground(adjustedPath, protectedId, null);
+ protectedId = UUID.randomUUID().toString();
--- End diff --
It can't be reversed. If it were reversed, the wrong ID would be passed. Maybe for clarity, a localProtectedId can be assigned and passed to findAndDeleteProtectedNodeInBackground()
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16054657
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ if ( protectedId != null )
+ {
+ /*
+ * CURATOR-79 - Handle an runtime exception's here and treat the
+ * same as a connection loss exception. This is necessary as, from
+ * the clients point of view, an exception has been thrown and the
+ * zNode should not exist on ZK. This was causing deadlock in the
+ * locking recipes.
+ */
+ findAndDeleteProtectedNodeInBackground(adjustedPath, protectedId, null);
+ protectedId = UUID.randomUUID().toString();
--- End diff --
Is there a possible race condition between issuing the background delete and reassigning the protectedId? I almost think we would want to swap the order. (Intuition only, no concerete examples of this).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16054205
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
--- End diff --
I think the 3 catch clauses can be combined for clarity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16083651
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
--- End diff --
I think it would be cleaner because there a duplicated block of code now. Or, move the duplicated code into a new method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by ortschun <gi...@git.apache.org>.
Github user ortschun commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16068759
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ if ( protectedId != null )
+ {
+ /*
+ * CURATOR-79 - Handle an runtime exception's here and treat the
--- End diff --
nit: could you fix the sentence, as something like "Handle runtime exceptions here"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/35#discussion_r16083287
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java ---
@@ -469,6 +469,26 @@ private String protectedPathInForeground(String adjustedPath, byte[] data) throw
}
throw e;
}
+ catch ( KeeperException e )
+ {
+ throw e;
+ }
+ catch ( Exception e )
+ {
+ if ( protectedId != null )
+ {
+ /*
+ * CURATOR-79 - Handle an runtime exception's here and treat the
+ * same as a connection loss exception. This is necessary as, from
+ * the clients point of view, an exception has been thrown and the
+ * zNode should not exist on ZK. This was causing deadlock in the
+ * locking recipes.
+ */
+ findAndDeleteProtectedNodeInBackground(adjustedPath, protectedId, null);
+ protectedId = UUID.randomUUID().toString();
--- End diff --
I don't think there's a race condition given that the findAndDeleteProtectionNodeInBackground is being passed a reference to the ID it needs to delete. I will change to assign to a local variable and pass this to the findAndDeleteProtectionNodeInBackground method for clarity.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: CURATOR-79 - Modified the 'withProtection' h...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/35#issuecomment-52836154
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---