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

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

@@ -17,6 +17,8 @@
  */
 package org.apache.hadoop.util;
 
+import java.util.concurrent.atomic.AtomicReference;
+
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 import org.slf4j.Logger;
@@ -36,8 +38,10 @@ public final class ExitUtil {
       LOG = LoggerFactory.getLogger(ExitUtil.class.getName());
   private static volatile boolean systemExitDisabled = false;
   private static volatile boolean systemHaltDisabled = false;
-  private static volatile ExitException firstExitException;
-  private static volatile HaltException firstHaltException;
+  private static final AtomicReference<ExitException> FIRST_EXIT_EXCEPTION =
+      new AtomicReference<>();
+  private static final AtomicReference<HaltException> FIRST_HALT_EXCEPTION =
+      new AtomicReference<>();
   /** Message raised from an exit exception if none were provided: {@value}. */
   public static final String EXIT_EXCEPTION_MESSAGE = "ExitException";
   /** Message raised from a halt exception if none were provided: {@value}. */
@@ -159,92 +163,165 @@ public static void disableSystemHalt() {
    */
   public static boolean terminateCalled() {
     // Either we set this member or we actually called System#exit
-    return firstExitException != null;
+    return FIRST_EXIT_EXCEPTION.get() != null;
   }
 
   /**
    * @return true if halt has been called.
    */
   public static boolean haltCalled() {
-    return firstHaltException != null;
+    // Either we set this member or we actually called Runtime#halt
+    return FIRST_HALT_EXCEPTION.get() != null;
   }
 
   /**
-   * @return the first ExitException thrown, null if none thrown yet.
+   * @return the first {@code ExitException} thrown, null if none thrown yet.
    */
   public static ExitException getFirstExitException() {
-    return firstExitException;
+    return FIRST_EXIT_EXCEPTION.get();
   }
 
   /**
    * @return the first {@code HaltException} thrown, null if none thrown yet.
    */
   public static HaltException getFirstHaltException() {
-    return firstHaltException;
+    return FIRST_HALT_EXCEPTION.get();
   }
 
   /**
    * Reset the tracking of process termination. This is for use in unit tests
    * where one test in the suite expects an exit but others do not.
    */
   public static void resetFirstExitException() {
-    firstExitException = null;
+    FIRST_EXIT_EXCEPTION.set(null);
   }
 
+  /**
+   * Reset the tracking of process termination. This is for use in unit tests
+   * where one test in the suite expects a halt but others do not.
+   */
   public static void resetFirstHaltException() {
-    firstHaltException = null;
+    FIRST_HALT_EXCEPTION.set(null);
   }
 
   /**
+   * 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,
    * or, if system exits are disabled, rethrow the exception.
    * @param ee exit exception
+   * @throws ExitException if {@link System#exit(int)} is disabled and not suppressed by an Error
+   * @throws Error if {@link System#exit(int)} is disabled and one Error arise, suppressing
+   * anything else, even <code>ee</code>
    */
-  public static synchronized void terminate(ExitException ee)
+  public static void terminate(ExitException ee)
       throws ExitException {
-    int status = ee.getExitCode();
-    String msg = ee.getMessage();
+    final int status = ee.getExitCode();
+    Error caught = null;
     if (status != 0) {
-      //exit indicates a problem, log it
-      LOG.debug("Exiting with status {}: {}",  status, msg, ee);
-      LOG.info("Exiting with status {}: {}", status, msg);
+      try {
+        // exit indicates a problem, log it
+        String msg = ee.getMessage();
+        LOG.debug("Exiting with status {}: {}",  status, msg, ee);
+        LOG.info("Exiting with status {}: {}", status, msg);
+      } catch (Error e) {
+        // errors have higher priority than HaltException, it may be re-thrown.
+        // OOM and ThreadDeath are 2 examples of Errors to re-throw
+        caught = e;
+      } catch (Throwable t) {
+        // all other kind of throwables are suppressed
+        if (ee != t) {
+          ee.addSuppressed(t);
+        }
+      }
     }
     if (systemExitDisabled) {
-      LOG.error("Terminate called", ee);
-      if (!terminateCalled()) {
-        firstExitException = ee;
+      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);
+        }
+      } catch (Throwable t) {
+        // all other kind of throwables are suppressed
+        if (ee != t) {
+          ee.addSuppressed(t);
+        }
+      }
+      FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
+      if (caught != null) {
+        caught.addSuppressed(ee);
+        throw caught;
       }
       throw ee;
+    } else {
+      // when exit is enabled, whatever Throwable happened, we exit the VM
+      System.exit(status);
     }
-    System.exit(status);
   }
 
   /**
-   * Forcibly terminates the currently running Java virtual machine.
-   * The exception argument is rethrown if JVM halting is disabled.
-   * @param ee the exception containing the status code, message and any stack
+   * Halts the JVM if halt is enabled, rethrow provided exception or any raised error otherwise.
+   * If halt is disabled, this method throws either the exception argument if no
+   * error arise, the first error if at least one arise, suppressing <code>he</code>.
+   * If halt is enabled, all throwables are caught, even errors.
+   *
+   * @param he the exception containing the status code, message and any stack
    * trace.
-   * @throws HaltException if {@link Runtime#halt(int)} is disabled.
+   * @throws HaltException if {@link Runtime#halt(int)} is disabled and not suppressed by an Error
+   * @throws Error if {@link Runtime#halt(int)} is disabled and one Error arise, suppressing
+   * anyuthing else, even <code>he</code>
    */
-  public static synchronized void halt(HaltException ee) throws HaltException {
-    int status = ee.getExitCode();
-    String msg = ee.getMessage();
-    try {
-      if (status != 0) {
-        //exit indicates a problem, log it
-        LOG.info("Halt with status {}: {}", status, msg, ee);
+  public static void halt(HaltException he) throws HaltException {
+    final int status = he.getExitCode();
+    Error caught = null;
+    if (status != 0) {
+      try {
+        // exit indicates a problem, log it
+        String msg = he.getMessage();
+        LOG.info("Halt with status {}: {}", status, msg, he);
+      } catch (Error e) {
+        // errors have higher priority than HaltException, it may be re-thrown.
+        // OOM and ThreadDeath are 2 examples of Errors to re-throw
+        caught = e;
+      } catch (Throwable t) {
+        // all other kind of throwables are suppressed
+        if (he != t) {
+          he.addSuppressed(t);
+        }
       }
-    } catch (Exception ignored) {
-      // ignore exceptions here, as it may be due to an out of memory situation
     }
+    // systemHaltDisabled is volatile and not used in scenario nheding atomicty,
+    // thus it does not nhed a synchronized access nor a atomic access
     if (systemHaltDisabled) {
-      LOG.error("Halt called", ee);
-      if (!haltCalled()) {
-        firstHaltException = ee;
+      try {
+        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);
+        }
+      } catch (Throwable t) {
+        // all other kind of throwables are suppressed
+        if (he != t) {
+          he.addSuppressed(t);
+        }
       }
-      throw ee;
+      FIRST_HALT_EXCEPTION.compareAndSet(null, he);
+      if (caught != null) {
+        caught.addSuppressed(he);
+        throw caught;
+      }
+      // not suppressed by a higher prority error
+      throw he;
+    } else {
+      // when halt is enabled, whatever Throwable happened, we halt the VM
+      Runtime.getRuntime().halt(status);
     }
-    Runtime.getRuntime().halt(status);
   }
 
   /**

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

@@ -0,0 +1,122 @@
+/**
+ * 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.util;
+
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.Test;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+
+public class TestExitUtil {
+
+  @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());
+
+      // 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 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();
+    }
+  }
+
+  @Test
+  public void testGetSetHaltExceptions() throws Throwable {
+    // prepare states and exceptions
+    ExitUtil.disableSystemHalt();
+    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());
+
+      // 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();
+    }
+  }
+}