HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…

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

@@ -204,6 +204,20 @@ public static void resetFirstHaltException() {
     FIRST_HALT_EXCEPTION.set(null);
   }
 
+  /**
+   * Suppresses if legit and returns the first non-null of the two. Legit means
+   * <code>suppressor</code> if neither <code>null</code> nor <code>suppressed</code>.
+   */
+  private static <T extends Throwable> T addSuppressed(T suppressor, T suppressed) {
+    if (suppressor == null) {
+      return suppressed;
+    }
+    if (suppressor != suppressed) {
+      suppressor.addSuppressed(suppressed);
+    }
+    return suppressor;
+  }
+
   /**
    * Exits the JVM if exit is enabled, rethrow provided exception or any raised error otherwise.
    * Inner termination: either exit with the exception's exit code,
@@ -213,8 +227,7 @@ public static void resetFirstHaltException() {
    * @throws Error if {@link System#exit(int)} is disabled and one Error arise, suppressing
    * anything else, even <code>ee</code>
    */
-  public static void terminate(ExitException ee)
-      throws ExitException {
+  public static void terminate(final ExitException ee) throws ExitException {
     final int status = ee.getExitCode();
     Error caught = null;
     if (status != 0) {
@@ -229,32 +242,25 @@ public static void terminate(ExitException ee)
         caught = e;
       } catch (Throwable t) {
         // all other kind of throwables are suppressed
-        if (ee != t) {
-          ee.addSuppressed(t);
-        }
+        addSuppressed(ee, t);
       }
     }
     if (systemExitDisabled) {
       try {
         LOG.error("Terminate called", ee);
       } catch (Error e) {
         // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
-        if (caught == null) {
-          caught = e;
-        } else if (caught != e) {
-          caught.addSuppressed(e);
-        }
+        caught = addSuppressed(caught, e);
       } catch (Throwable t) {
         // all other kind of throwables are suppressed
-        if (ee != t) {
-          ee.addSuppressed(t);
-        }
+        addSuppressed(ee, t);
       }
       FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
       if (caught != null) {
         caught.addSuppressed(ee);
         throw caught;
       }
+      // not suppressed by a higher prority error
       throw ee;
     } else {
       // when exit is enabled, whatever Throwable happened, we exit the VM
@@ -274,7 +280,7 @@ public static void terminate(ExitException ee)
    * @throws Error if {@link Runtime#halt(int)} is disabled and one Error arise, suppressing
    * anyuthing else, even <code>he</code>
    */
-  public static void halt(HaltException he) throws HaltException {
+  public static void halt(final HaltException he) throws HaltException {
     final int status = he.getExitCode();
     Error caught = null;
     if (status != 0) {
@@ -288,9 +294,7 @@ public static void halt(HaltException he) throws HaltException {
         caught = e;
       } catch (Throwable t) {
         // all other kind of throwables are suppressed
-        if (he != t) {
-          he.addSuppressed(t);
-        }
+        addSuppressed(he, t);
       }
     }
     // systemHaltDisabled is volatile and not used in scenario nheding atomicty,
@@ -300,16 +304,10 @@ public static void halt(HaltException he) throws HaltException {
         LOG.error("Halt called", he);
       } catch (Error e) {
         // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
-        if (caught == null) {
-          caught = e;
-        } else if (caught != e) {
-          caught.addSuppressed(e);
-        }
+        caught = addSuppressed(caught, e);
       } catch (Throwable t) {
         // all other kind of throwables are suppressed
-        if (he != t) {
-          he.addSuppressed(t);
-        }
+        addSuppressed(he, t);
       }
       FIRST_HALT_EXCEPTION.compareAndSet(null, he);
       if (caught != null) {

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

@@ -23,57 +23,66 @@
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 
+import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 
 import org.apache.hadoop.util.ExitUtil.ExitException;
 import org.apache.hadoop.util.ExitUtil.HaltException;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
 
+public class TestExitUtil extends AbstractHadoopTestBase {
 
-public class TestExitUtil {
+  @Before
+  public void before() {
+    ExitUtil.disableSystemExit();
+    ExitUtil.disableSystemHalt();
+    ExitUtil.resetFirstExitException();
+    ExitUtil.resetFirstHaltException();
+  }
+
+  @After
+  public void after() {
+    ExitUtil.resetFirstExitException();
+    ExitUtil.resetFirstHaltException();
+  }
 
   @Test
   public void testGetSetExitExceptions() throws Throwable {
     // prepare states and exceptions
-    ExitUtil.disableSystemExit();
-    ExitUtil.resetFirstExitException();
     ExitException ee1 = new ExitException(1, "TestExitUtil forged 1st ExitException");
     ExitException ee2 = new ExitException(2, "TestExitUtil forged 2nd ExitException");
-    try {
-      // check proper initial settings
-      assertFalse("ExitUtil.terminateCalled initial value should be false",
-          ExitUtil.terminateCalled());
-      assertNull("ExitUtil.getFirstExitException initial value should be null",
-          ExitUtil.getFirstExitException());
+    // check proper initial settings
+    assertFalse("ExitUtil.terminateCalled initial value should be false",
+        ExitUtil.terminateCalled());
+    assertNull("ExitUtil.getFirstExitException initial value should be null",
+        ExitUtil.getFirstExitException());
 
-      // simulate/check 1st call
-      ExitException ee = intercept(ExitException.class, ()->ExitUtil.terminate(ee1));
-      assertSame("ExitUtil.terminate should have rethrown its ExitException argument but it "
-          + "had thrown something else", ee1, ee);
-      assertTrue("ExitUtil.terminateCalled should be true after 1st ExitUtil.terminate call",
-          ExitUtil.terminateCalled());
-      assertSame("ExitUtil.terminate should store its 1st call's ExitException",
-          ee1, ExitUtil.getFirstExitException());
+    // simulate/check 1st call
+    ExitException ee = intercept(ExitException.class, ()->ExitUtil.terminate(ee1));
+    assertSame("ExitUtil.terminate should have rethrown its ExitException argument but it "
+        + "had thrown something else", ee1, ee);
+    assertTrue("ExitUtil.terminateCalled should be true after 1st ExitUtil.terminate call",
+        ExitUtil.terminateCalled());
+    assertSame("ExitUtil.terminate should store its 1st call's ExitException",
+        ee1, ExitUtil.getFirstExitException());
 
-      // simulate/check 2nd call not overwritting 1st one
-      ee = intercept(ExitException.class, ()->ExitUtil.terminate(ee2));
-      assertSame("ExitUtil.terminate should have rethrown its HaltException argument but it "
-          + "had thrown something else", ee2, ee);
-      assertTrue("ExitUtil.terminateCalled should still be true after 2nd ExitUtil.terminate call",
-          ExitUtil.terminateCalled());
-      // 2nd call rethrown the 2nd ExitException yet only the 1st only should have been stored
-      assertSame("ExitUtil.terminate when called twice should only remember 1st call's "
-          + "ExitException", ee1, ExitUtil.getFirstExitException());
+    // simulate/check 2nd call not overwritting 1st one
+    ee = intercept(ExitException.class, ()->ExitUtil.terminate(ee2));
+    assertSame("ExitUtil.terminate should have rethrown its HaltException argument but it "
+        + "had thrown something else", ee2, ee);
+    assertTrue("ExitUtil.terminateCalled should still be true after 2nd ExitUtil.terminate call",
+        ExitUtil.terminateCalled());
+    // 2nd call rethrown the 2nd ExitException yet only the 1st only should have been stored
+    assertSame("ExitUtil.terminate when called twice should only remember 1st call's "
+        + "ExitException", ee1, ExitUtil.getFirstExitException());
 
-      // simulate cleanup, also tries to make sure state is ok for all junit still has to do
-      ExitUtil.resetFirstExitException();
-      assertFalse("ExitUtil.terminateCalled should be false after "
-          + "ExitUtil.resetFirstExitException call", ExitUtil.terminateCalled());
-      assertNull("ExitUtil.getFirstExitException should be null after "
-          + "ExitUtil.resetFirstExitException call", ExitUtil.getFirstExitException());
-    } finally {
-      // cleanup
-      ExitUtil.resetFirstExitException();
-    }
+    // simulate cleanup, also tries to make sure state is ok for all junit still has to do
+    ExitUtil.resetFirstExitException();
+    assertFalse("ExitUtil.terminateCalled should be false after "
+        + "ExitUtil.resetFirstExitException call", ExitUtil.terminateCalled());
+    assertNull("ExitUtil.getFirstExitException should be null after "
+        + "ExitUtil.resetFirstExitException call", ExitUtil.getFirstExitException());
   }
 
   @Test
@@ -83,40 +92,36 @@ public void testGetSetHaltExceptions() throws Throwable {
     ExitUtil.resetFirstHaltException();
     HaltException he1 = new HaltException(1, "TestExitUtil forged 1st HaltException");
     HaltException he2 = new HaltException(2, "TestExitUtil forged 2nd HaltException");
-    try {
-      // check proper initial settings
-      assertFalse("ExitUtil.haltCalled initial value should be false",
-          ExitUtil.haltCalled());
-      assertNull("ExitUtil.getFirstHaltException initial value should be null",
-          ExitUtil.getFirstHaltException());
 
-      // simulate/check 1st call
-      HaltException he = intercept(HaltException.class, ()->ExitUtil.halt(he1));
-      assertSame("ExitUtil.halt should have rethrown its HaltException argument but it had "
-          +"thrown something else", he1, he);
-      assertTrue("ExitUtil.haltCalled should be true after 1st ExitUtil.halt call",
-          ExitUtil.haltCalled());
-      assertSame("ExitUtil.halt should store its 1st call's HaltException",
-          he1, ExitUtil.getFirstHaltException());
+    // check proper initial settings
+    assertFalse("ExitUtil.haltCalled initial value should be false",
+        ExitUtil.haltCalled());
+    assertNull("ExitUtil.getFirstHaltException initial value should be null",
+        ExitUtil.getFirstHaltException());
+
+    // simulate/check 1st call
+    HaltException he = intercept(HaltException.class, ()->ExitUtil.halt(he1));
+    assertSame("ExitUtil.halt should have rethrown its HaltException argument but it had "
+        +"thrown something else", he1, he);
+    assertTrue("ExitUtil.haltCalled should be true after 1st ExitUtil.halt call",
+        ExitUtil.haltCalled());
+    assertSame("ExitUtil.halt should store its 1st call's HaltException",
+        he1, ExitUtil.getFirstHaltException());
 
-      // simulate/check 2nd call not overwritting 1st one
-      he = intercept(HaltException.class, ()->ExitUtil.halt(he2));
-      assertSame("ExitUtil.halt should have rethrown its HaltException argument but it had "
-          +"thrown something else", he2, he);
-      assertTrue("ExitUtil.haltCalled should still be true after 2nd ExitUtil.halt call",
-          ExitUtil.haltCalled());
-      assertSame("ExitUtil.halt when called twice should only remember 1st call's HaltException",
-          he1, ExitUtil.getFirstHaltException());
+    // simulate/check 2nd call not overwritting 1st one
+    he = intercept(HaltException.class, ()->ExitUtil.halt(he2));
+    assertSame("ExitUtil.halt should have rethrown its HaltException argument but it had "
+        +"thrown something else", he2, he);
+    assertTrue("ExitUtil.haltCalled should still be true after 2nd ExitUtil.halt call",
+        ExitUtil.haltCalled());
+    assertSame("ExitUtil.halt when called twice should only remember 1st call's HaltException",
+        he1, ExitUtil.getFirstHaltException());
 
-      // simulate cleanup, also tries to make sure state is ok for all junit still has to do
-      ExitUtil.resetFirstHaltException();
-      assertFalse("ExitUtil.haltCalled should be false after "
-          + "ExitUtil.resetFirstHaltException call", ExitUtil.haltCalled());
-      assertNull("ExitUtil.getFirstHaltException should be null after "
-          + "ExitUtil.resetFirstHaltException call", ExitUtil.getFirstHaltException());
-    } finally {
-      // cleanup, useless if last test succeed, useful if not
-      ExitUtil.resetFirstHaltException();
-    }
+    // simulate cleanup, also tries to make sure state is ok for all junit still has to do
+    ExitUtil.resetFirstHaltException();
+    assertFalse("ExitUtil.haltCalled should be false after "
+        + "ExitUtil.resetFirstHaltException call", ExitUtil.haltCalled());
+    assertNull("ExitUtil.getFirstHaltException should be null after "
+        + "ExitUtil.resetFirstHaltException call", ExitUtil.getFirstHaltException());
   }
 }