[GitHub] [curator] tisonkun commented on a diff in pull request #446: CURATOR-518: Fix LeaderSelector requeue broken by interruptLeadership

["tisonkun (via GitHub)" <gi...@apache.org>]

tisonkun commented on code in PR #446:
URL: https://github.com/apache/curator/pull/446#discussion_r1125432578


##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderSelector.java:
##########
@@ -379,10 +423,31 @@ public boolean hasLeadership()
      */
     public synchronized void interruptLeadership()
     {
-        Future<?> task = ourTask.get();
-        if ( task != null )
+        // Correctness with requeue:
+        // * requeue, interrupt and ourTask are guarded by synchronized(this), so requeue is not
+        //   able to execute interleaving.
+        // * hasLeadership is true, so we are not cancelling before task execution.
+        // * auto-requeue is always executed before task termination and has no interruption point.
+        // * it is harmless to interrupt nearly completed task(e.g. lost or release leadership).
+        if ( ourTask != null && hasLeadership )
         {
-            task.cancel(true);
+            ourTask.cancel(true);
+        }
+    }
+
+    /**
+     * Cancel ongoing election and leadership.
+     *
+     * <p>Subsequent {@link #requeue()} will succeed if {@link #autoRequeue()} is not enabled.
+     */
+    private synchronized void cancelElection() {
+        if ( ourTask != null ) {
+            ourTask.cancel(true);
+            isQueued = false;

Review Comment:
   ```suggestion
               clearIsQueued();
   ```
   
   Keep consistent.



##########
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderSelector.java:
##########
@@ -237,13 +242,15 @@ private synchronized boolean internalRequeue()
         if ( !isQueued && (state.get() == State.STARTED) )
         {
             isQueued = true;
-            Future<Void> task = executorService.submit(new Callable<Void>()
+            Future<?> oldTask = ourTask;
+            ourTask = executorService.submit(new Callable<Void>()
             {
                 @Override
                 public Void call() throws Exception
                 {
                     try
                     {
+                        waitTaskDone(oldTask, Duration.ofMillis(500), Duration.ofSeconds(5));

Review Comment:
   It seems this is a best-effort waiting call and we don't have happens-before relation to ensure that the previous task must be done or cancelled.
   
   Filter only `hasLeadership` will proceed `interruptLeadership` seems enough for fixing 518?



-- 
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