HADOOP-16341. ShutDownHookManager: Regressed performance on Hook removals after HADOOP-15679

Author: zeroflag

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ShutdownHookManager.java

@@ -254,7 +254,9 @@ TimeUnit getTimeUnit() {
   private AtomicBoolean shutdownInProgress = new AtomicBoolean(false);
 
   //private to constructor to ensure singularity
-  private ShutdownHookManager() {
+  @VisibleForTesting
+  @InterfaceAudience.Private
+  ShutdownHookManager() {
   }
 
   /**
@@ -267,8 +269,8 @@ private ShutdownHookManager() {
   @VisibleForTesting
   List<HookEntry> getShutdownHooksInOrder() {
     List<HookEntry> list;
-    synchronized (MGR.hooks) {
-      list = new ArrayList<>(MGR.hooks);
+    synchronized (hooks) {
+      list = new ArrayList<>(hooks);
     }
     Collections.sort(list, new Comparator<HookEntry>() {
 
@@ -342,7 +344,9 @@ public boolean removeShutdownHook(Runnable shutdownHook) {
       throw new IllegalStateException("Shutdown in progress, cannot remove a " +
           "shutdownHook");
     }
-    return hooks.remove(new HookEntry(shutdownHook, 0));
+    // hooks are only == by runnable
+    return hooks.remove(new HookEntry(shutdownHook, 0, TIMEOUT_MINIMUM,
+      TIME_UNIT_DEFAULT));
   }
 
   /**
@@ -354,7 +358,8 @@ public boolean removeShutdownHook(Runnable shutdownHook) {
   @InterfaceAudience.Public
   @InterfaceStability.Stable
   public boolean hasShutdownHook(Runnable shutdownHook) {
-    return hooks.contains(new HookEntry(shutdownHook, 0));
+    return hooks.contains(new HookEntry(shutdownHook, 0, TIMEOUT_MINIMUM,
+      TIME_UNIT_DEFAULT));
   }
 
   /**

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestShutdownHookManager.java

@@ -43,13 +43,10 @@ public class TestShutdownHookManager {
       LoggerFactory.getLogger(TestShutdownHookManager.class.getName());
 
   /**
-   * remove all the shutdown hooks so that they never get invoked later
-   * on in this test process.
+   * A new instance of ShutdownHookManager to ensure parallel tests
+   * don't have shared context
    */
-  @After
-  public void clearShutdownHooks() {
-    ShutdownHookManager.get().clearShutdownHooks();
-  }
+  private final ShutdownHookManager mgr = new ShutdownHookManager();
 
   /**
    * Verify hook registration, then execute the hook callback stage
@@ -58,7 +55,6 @@ public void clearShutdownHooks() {
    */
   @Test
   public void shutdownHookManager() {
-    ShutdownHookManager mgr = ShutdownHookManager.get();
     assertNotNull("No ShutdownHookManager", mgr);
     assertEquals(0, mgr.getShutdownHooksInOrder().size());
     Hook hook1 = new Hook("hook1", 0, false);
@@ -193,7 +189,6 @@ public void testShutdownTimeoutBadConfiguration() throws Throwable {
    */
   @Test
   public void testDuplicateRegistration() throws Throwable {
-    ShutdownHookManager mgr = ShutdownHookManager.get();
     Hook hook = new Hook("hook1", 0, false);
 
     // add the hook
@@ -222,6 +217,20 @@ public void testDuplicateRegistration() throws Throwable {
 
   }
 
+  @Test
+  public void testShutdownRemove() throws Throwable {
+    assertNotNull("No ShutdownHookManager", mgr);
+    assertEquals(0, mgr.getShutdownHooksInOrder().size());
+    Hook hook1 = new Hook("hook1", 0, false);
+    Hook hook2 = new Hook("hook2", 0, false);
+    mgr.addShutdownHook(hook1, 9); // create Hook1 with priority 9
+    assertTrue(mgr.hasShutdownHook(hook1)); // hook1 lookup works
+    assertEquals(1, mgr.getShutdownHooksInOrder().size()); // 1 hook
+    assertFalse(mgr.removeShutdownHook(hook2)); // can't delete hook2
+    assertTrue(mgr.removeShutdownHook(hook1)); // can delete hook1
+    assertEquals(0, mgr.getShutdownHooksInOrder().size()); // no more hooks
+  }
+
   private static final AtomicInteger INVOCATION_COUNT = new AtomicInteger();
 
   /**