HBASE-30502 ProcedureFutureUtil.suspendIfNecessary may skip wakingup …

hbase-server/src/main/java/org/apache/hadoop/hbase/procedure2/ProcedureFutureUtil.java

@@ -20,9 +20,9 @@
 import java.io.IOException;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
-import org.apache.commons.lang3.mutable.MutableBoolean;
 import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
 import org.apache.hadoop.hbase.util.FutureUtils;
 import org.apache.hadoop.hbase.util.IdLock;
@@ -60,8 +60,7 @@ public static void suspendIfNecessary(Procedure<?> proc,
     Consumer<CompletableFuture<Void>> setFuture, CompletableFuture<Void> future,
     MasterProcedureEnv env, Runnable actionAfterDone)
     throws IOException, ProcedureSuspendedException {
-    MutableBoolean completed = new MutableBoolean(false);
-    Thread currentThread = Thread.currentThread();
+    AtomicInteger completed = new AtomicInteger(0);
     // This is for testing. In ProcedureTestingUtility, we will restart a ProcedureExecutor and
     // reuse it, for performance, so we need to make sure that all the procedure have been stopped.
     // But here, the callback of this future is not executed in a PEWorker, so in ProcedureExecutor
@@ -71,12 +70,11 @@ public static void suspendIfNecessary(Procedure<?> proc,
     // asyncTaskExecutor will also be shutdown so we can not add anything back to the scheduler.
     ExecutorService asyncTaskExecutor = env.getAsyncTaskExecutor();
     FutureUtils.addListener(future, (r, e) -> {
-      if (Thread.currentThread() == currentThread) {
+      if (completed.compareAndSet(0, 1)) {
+        // this means the future has already been completed, as we have not execute the below
+        // compareAndSet(0, 2) yet, so here we just set completed to true without doing anything
         LOG.debug("The future has completed while adding callback, give up suspending procedure {}",
           proc);
-        // this means the future has already been completed, as we call the callback directly while
-        // calling addListener, so here we just set completed to true without doing anything
-        completed.setTrue();
         return;
       }
       LOG.debug("Going to wake up procedure {} because future has completed", proc);
@@ -88,14 +86,15 @@ public static void suspendIfNecessary(Procedure<?> proc,
       // I/O, which could even cause dead lock.
       asyncTaskExecutor.execute(() -> wakeUp(proc, env));
     });
-    if (completed.getValue()) {
-      FutureUtils.get(future);
-      actionAfterDone.run();
-    } else {
+    if (completed.compareAndSet(0, 2)) {
       // suspend the procedure
       setFuture.accept(future);
-      proc.skipPersistence();
       suspend(proc);
+    } else {
+      // if completed has been set to 1, i.e, not 0, it means the future has already been completed,
+      // so we give up suspending and call the done action directly.
+      FutureUtils.get(future);
+      actionAfterDone.run();
     }
   }
 

hbase-server/src/test/java/org/apache/hadoop/hbase/procedure2/TestProcedureFutureUtil.java

@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.procedure2;
+
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import org.apache.commons.lang3.mutable.MutableObject;
+import org.apache.hadoop.hbase.master.procedure.MasterProcedureEnv;
+import org.apache.hadoop.hbase.testclassification.MasterTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(SmallTests.TAG)
+@Tag(MasterTests.TAG)
+public class TestProcedureFutureUtil {
+
+  @Test
+  public void testCompleteInSameThread() throws IOException {
+    Procedure<?> proc = mock(Procedure.class);
+    MutableObject<CompletableFuture<Void>> setFuture = new MutableObject<>();
+
+    CompletableFuture<Void> future = new CompletableFuture<>();
+
+    MasterProcedureEnv env = mock(MasterProcedureEnv.class);
+    ExecutorService exec = mock(ExecutorService.class);
+    when(env.getAsyncTaskExecutor()).thenReturn(exec);
+
+    Runnable actionAfterDone = mock(Runnable.class);
+
+    assertThrows(ProcedureSuspendedException.class, () -> ProcedureFutureUtil
+      .suspendIfNecessary(proc, setFuture::setValue, future, env, actionAfterDone));
+
+    assertSame(future, setFuture.get());
+    verify(proc, times(1)).skipPersistence();
+
+    // complete in the same thread, and make sure the callback has been scheduled correctly
+    future.complete(null);
+    verify(env, times(1)).getAsyncTaskExecutor();
+    verify(exec, times(1)).execute(any());
+  }
+}