[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…

[GitBox <gi...@apache.org>]

steveloughran commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r862765210


##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -226,25 +251,48 @@ public static synchronized void terminate(ExitException ee)
    * trace.
    * @throws HaltException if {@link Runtime#halt(int)} is disabled.
    */
-  public static synchronized void halt(HaltException ee) throws HaltException {
-    int status = ee.getExitCode();
-    String msg = ee.getMessage();
-    try {
-      if (status != 0) {
+  public static void halt(HaltException ee) throws HaltException {
+    final int status = ee.getExitCode();
+    Error catched = null;
+    if (status != 0) {
+      try {
         //exit indicates a problem, log it
+        String msg = ee.getMessage();
         LOG.info("Halt with status {}: {}", status, msg, ee);
+      } catch (Error e) {
+        catched = e; // errors have higher priority than HaltException, it may be re-thrown. OOM and ThreadDeath are 2 examples of Errors to re-throw
+      } catch (Throwable t) {
+        // all other kind of throwables are supressed
+        ee.addSuppressed(t);
       }
-    } catch (Exception ignored) {
-      // ignore exceptions here, as it may be due to an out of memory situation
     }
-    if (systemHaltDisabled) {
-      LOG.error("Halt called", ee);
-      if (!haltCalled()) {
-        firstHaltException = ee;
+    if (systemHaltDisabled) { // this is a volatile so reading it does not need a synchronized block

Review Comment:
   do think we should move to atomic boolean? it's a bit less efficient, but this isn't a performance codepath, and it might make for safer code for future maintenance. there are details about volatile that nobody ever remembers/knows



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -200,23 +200,48 @@ public static void resetFirstHaltException() {
    * or, if system exits are disabled, rethrow the exception.
    * @param ee exit exception
    */
-  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 catched = null;

Review Comment:
   `caught`



##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -200,23 +200,48 @@ public static void resetFirstHaltException() {
    * or, if system exits are disabled, rethrow the exception.
    * @param ee exit exception
    */
-  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 catched = 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) {
+        catched = e; // errors have higher priority than HaltException, it may be re-thrown. OOM and ThreadDeath are 2 examples of Errors to re-throw

Review Comment:
   nit. put comment on line above



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org