[GitHub] [curator] XComp commented on a diff in pull request #430: CURATOR-644. CURATOR-645. Fix livelock in LeaderLatch

[GitBox <gi...@apache.org>]

XComp commented on code in PR #430:
URL: https://github.com/apache/curator/pull/430#discussion_r957341820


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
             {
                 try
                 {
-                    if ( client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED) || !hasLeadership.get() )

Review Comment:
   I'm wondering whether we should remove this `if` entirely: Let's assume the current instance doesn't have the leadership but watches the child node referring to the current leader. If the connection was temporarily suspended it could be that the actual leader lost its leadership in the meantime. In that case, we want the current instance to check the leadership. :thinking: 



##########
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java:
##########
@@ -218,6 +218,30 @@ public void testWatchedNodeDeletedOnReconnect() throws Exception
         }
     }
 
+    @Test
+    public void testOurPathDeletedOnReconnect() throws Exception

Review Comment:
   The test is not suitable for the change. I reverted the change and ran the test. The test still succeeded.



##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -667,9 +670,9 @@ protected void handleStateChange(ConnectionState newState)
             {
                 try
                 {
-                    if ( client.getConnectionStateErrorPolicy().isErrorState(ConnectionState.SUSPENDED) || !hasLeadership.get() )

Review Comment:
   But thinking about it once more, this might not be an issue because the current LeaderLatch would have a watcher on the current leader's node which would have been triggered when the leader's child would have been deleted. :thinking: 



##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java:
##########
@@ -604,7 +604,7 @@ else if ( ourIndex == 0 )
                 @Override
                 public void process(WatchedEvent event)
                 {
-                    if ( (state.get() == State.STARTED) && (event.getType() == Event.EventType.NodeDeleted) && (localOurPath != null) )

Review Comment:
   Good point. Due to the condition in [LeaderLatch:588](https://github.com/apache/curator/blob/425598cb6bf6a5b227a5fdd293fe46c7978d6578/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L588), `ourIndex` must be `-1` if `localOurPath` is actually `null` and, therefore, would prevent the execution from reaching the `else` branch (instead the if condition in [LeaderLatch:592](https://github.com/apache/curator/blob/425598cb6bf6a5b227a5fdd293fe46c7978d6578/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L592) applies) where the watcher is set.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org