You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/05/02 08:39:22 UTC
[GitHub] [hadoop] HerCath opened a new pull request, #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
HerCath opened a new pull request, #4255:
URL: https://github.com/apache/hadoop/pull/4255
…king halt + enlarged catches (robustness) to all Throwables (not just Exceptions)
<!--
Thanks for sending a pull request!
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute
2. Make sure your PR title starts with JIRA issue id, e.g., 'HADOOP-17799. Your PR title ...'.
-->
### Description of PR
I've reduced the synchronized blocks scope so System.exit and Runtime.halt calls aren't within their boundaries, so ExitUtil wrappers do not block each others (System.exit never returns if called and no SecurityException is raised, so ExitUtil.terminate was never releasing the acquired lock, thus forbidding ExitUtil.halt to be able to halt the JVM even when in the middle of a graceful shutdown, thus it was not behaving like the 2 wrapped java's methods System.exit/Runtime.halt which do not block each other)
I've altered throwable handling:
- what is catched: was nothing or only Exception, now all Throwables are catched (even ThreadDeath)
- what is rethrown: when exit/halt has been disabled, if what was catched is an Error it will be rethrown rather than the initial ExitException/HaltException. Other Throwables will be added as suppressed to the Exit/HaltException
- what wasn't catched: if not disabled, even is something was raised that wasn't catched before, it is now catched and System.exit/Runtime.halt is always called
- what is suppressed: if the what needs to be rethrown is changed on the way, the newly to-be-thrown will have the old one as a suppressed Throwable. I've also done this for the Exit/Halt Exception that can supress Throwables that are not Error (might not be a so good idea)
### How was this patch tested?
No more tests than the existing ones (if any). This case is not really hard to reproduce but the test would need to exit a JVM. I've not added such tests because if unit does not fork, it would kills the test suite (thus impacting all tests). I think developing a robust test for this specific case is way more hard and dangerous to offset the cost of a review, the risk of what could be missed by this review.
Easiest way can be reproduced the initial bug: having a shutdown hook call ExitUtil.terminate, have another thread that will call ExitUtil.halt after (use pauses to ensure it calls it after the hook), witness the JVM not stopping and needing either an external kill or a internal Runtime.halt call, maybe check the JVM threads' stacks too to view the ExitUtil.terminate call stuck on System.exit, and ExitUtil.halt call stuck on ExitUtil.terminate.
### For code changes:
- [x] Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
- [ ] Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE`, `LICENSE-binary`, `NOTICE-binary` files?
--
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
[GitHub] [hadoop] ashutoshcipher commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
ashutoshcipher commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1176875950
Hi @HerCath Thanks for the PR. Did you get a chance to address the comments from @steveloughran ? Seems to be pending for a while. If you busy, I will happy to make some progress.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1125491011
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 53s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 42m 52s | | trunk passed |
| +1 :green_heart: | compile | 25m 1s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 21m 31s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 30s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 57s | | trunk passed |
| -1 :x: | javadoc | 1m 34s | [/branch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/2/artifact/out/branch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt) | hadoop-common in trunk failed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1. |
| +1 :green_heart: | javadoc | 1m 54s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 4s | | trunk passed |
| +1 :green_heart: | shadedclient | 25m 48s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 8s | | the patch passed |
| +1 :green_heart: | compile | 24m 2s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 24m 2s | | the patch passed |
| +1 :green_heart: | compile | 21m 41s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 21m 41s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 24s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 55s | | the patch passed |
| -1 :x: | javadoc | 1m 28s | [/patch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/2/artifact/out/patch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt) | hadoop-common in the patch failed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1. |
| +1 :green_heart: | javadoc | 2m 1s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 5s | | the patch passed |
| +1 :green_heart: | shadedclient | 25m 46s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 10s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 16s | | The patch does not generate ASF License warnings. |
| | | 228m 29s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4255 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 21a14d2e3f09 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / f279d644ee91e335c63cd090baf0635edc325655 |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/2/testReport/ |
| Max. process+thread count | 3135 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/2/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918150727
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -159,138 +163,155 @@ 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 void terminate(ExitException ee)
throws ExitException {
final int status = ee.getExitCode();
- Error catched = null;
+ Error caught = null;
if (status != 0) {
try {
- //exit indicates a problem, log it
+ // 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
+ // 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 supressed
+ // all other kind of throwables are suppressed
ee.addSuppressed(t);
}
}
if (systemExitDisabled) {
try {
LOG.error("Terminate called", ee);
} catch (Error e) {
- if (catched == null) {
- catched = e; // errors will be re-thrown
+ // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
+ if (caught == null) {
+ caught = e;
} else {
- catched.addSuppressed(e); // 1st raised error has priority and will be re-thrown, so the 1st error supresses the 2nd
+ caught.addSuppressed(e);
}
} catch (Throwable t) {
- ee.addSuppressed(t); // all other kind of throwables are supressed
- }
- synchronized (ExitUtil.class) {
- if (!terminateCalled()) {
- firstExitException = ee;
- }
+ // all other kind of throwables are suppressed
+ ee.addSuppressed(t);
}
- if (catched != null) {
- catched.addSuppressed(ee);
- throw catched;
+ FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
+ if (caught != null) {
Review Comment:
> understanding of exception and any throwable in general is to never throw 2 times the same instance.
some classes cache a raised exception and raised later. see https://issues.apache.org/jira/browse/HADOOP-16785
(I do agree with the "elsewhere" claim, just want to be resilient to the mistake)
--
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
[GitHub] [hadoop] steveloughran merged pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran merged PR #4255:
URL: https://github.com/apache/hadoop/pull/4255
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1180826528
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 42s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 37m 42s | | trunk passed |
| +1 :green_heart: | compile | 23m 4s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 20m 35s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 48s | | trunk passed |
| +1 :green_heart: | mvnsite | 2m 13s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 48s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 22s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 13s | | trunk passed |
| +1 :green_heart: | shadedclient | 23m 13s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 5s | | the patch passed |
| +1 :green_heart: | compile | 22m 22s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 22m 22s | | the patch passed |
| +1 :green_heart: | compile | 20m 33s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 20m 33s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 40s | | the patch passed |
| +1 :green_heart: | mvnsite | 2m 12s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 39s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 23s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 12s | | the patch passed |
| +1 :green_heart: | shadedclient | 23m 31s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 39s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 30s | | The patch does not generate ASF License warnings. |
| | | 215m 2s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/3/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4255 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux fdf031eb0575 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 7d6468bf35af9f4b56bf2c8d0102852d0de73bc7 |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/3/testReport/ |
| Max. process+thread count | 2938 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/3/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] steveloughran commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1183115938
+cherrypicked to branch-3.3; ran new test suite and all is good.
--
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
[GitHub] [hadoop] HerCath commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1116496071
Hi,
Thanks for the reviews. I've started to adjust my code to integrate them.
Ok, I'll adjust code style about comment + rename the variable to caught. I'm also adjusting the javadoc to make it clear that in enable mode all throwables are caught, even errors (except maybe only ThreadDeath if it happen after the last try/catch). And in disable mode, the argument exception is re-throw suppressing other internally thrown throwables unless at least one is an error, in which case the 1st error one is rethrow, suppressing anything else. like before the patch: I don't document about RuntimeException subclasses or Error that could be thrown by System.exit/Runtime.halt.
About atomic versus volatile. The "disable" flags (for halt and exit) are not used in test-and-set pattern, nor any kind of do-several-actions that would have to be atomically done so volatile has enough thread sharing capabilities. The "first(Exit|Halt)Exception" fields would benefit from it though so I'm changing them to AtomicReference. It the "disable" flags were more tightly coupled with the "first" fields, then a synchronized would be needed to handle both field values and ensure they always describe a legit ("flag", "first") tuple but that's not the case here too: each can be resetted whatever the other value is. Moving to Atomic has also another benefit: it allows to remove all synchronized blocks on the ExitUtil class, so even if badly/evil code enters a synchronized blocks on ExitUtil.class and never leaves it, ExitUtil terminate/halt code will still be able run without blocking.
About tests : Since i'm changing to AtomicReference I will add some (i still have to check about maybe some existing one) but only when in disable mode.
When enable mode, I'm a bit afraid of the impact if I miss something in the tests. A real test would require to spawn an external JVM with a valid classpath that would need to call ExitUtil with expected codes and check its return code when it dies or kills it after a timeout (the expected codes need to differ from those due to a kill on timeout). If the test for some obscur reason really go wrong and do not kill the external JVM I don't want to impact other tests because of a hanging JVM. If the server/platform is supposed to have a long uptime, I don't want to leak spawned processes, making it an unstable/unreliable testing environment after several test runs. I don't know if such tests would need to be cross platform too. I've access only to windows and linux, not MacOS so i won't be able to check if I really kill the spawned JVM (i would use java.lang.Process which should be portable, even for the kill). I also don't know the impact on other developpers if they run tests on th
eir own machine.
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918149554
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
+ assertNull(ExitUtil.getFirstExitException());
+
+ // simulate/check 1st call
+ try {
+ ExitUtil.terminate(ee1);
Review Comment:
Ok. I've checked how other tests using intercept were coded. So now the test methods all declare to throw Throwable like them. Plus all the assertEquals are now assertSame ones.
--
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
[GitHub] [hadoop] HerCath commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1125844154
The JDK11 javadoc failures seems to all come from an InodeTree.java class this patch does not touch. The only 2 classes (ExitUtil.java and TestExitUtil.java) it is responsible for neither raise javadoc errors not warnings. But i may have miss-read the report :)
If i want to fix the javadoc issue on InodeTree i should open another JIRA/check one is not already working on it ? Also my last pushed commit triggered this last failed build, if another pull request for another JIRA fixes the InodeTree javadoc issue, will it also trigger another (hopefully good) build for this patch pull request ? (sorry I'm not new to git but very new to pull requests :/)
--
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
[GitHub] [hadoop] steveloughran commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1116084353
checkstyle is complaining about line length. move the comments to the lines above the source and all is good. we don't normally have comments of any kind on the same lines as code
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1114806690
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 17m 59s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 1s | | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 41m 26s | | trunk passed |
| +1 :green_heart: | compile | 25m 9s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 21m 50s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 45s | | trunk passed |
| +1 :green_heart: | mvnsite | 2m 7s | | trunk passed |
| -1 :x: | javadoc | 1m 38s | [/branch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/artifact/out/branch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt) | hadoop-common in trunk failed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1. |
| +1 :green_heart: | javadoc | 2m 9s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 24s | | trunk passed |
| +1 :green_heart: | shadedclient | 27m 18s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 10s | | the patch passed |
| +1 :green_heart: | compile | 24m 2s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 24m 2s | | the patch passed |
| +1 :green_heart: | compile | 21m 36s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 21m 36s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| -0 :warning: | checkstyle | 1m 23s | [/results-checkstyle-hadoop-common-project_hadoop-common.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/artifact/out/results-checkstyle-hadoop-common-project_hadoop-common.txt) | hadoop-common-project/hadoop-common: The patch generated 3 new + 2 unchanged - 0 fixed = 5 total (was 2) |
| +1 :green_heart: | mvnsite | 1m 55s | | the patch passed |
| -1 :x: | javadoc | 1m 26s | [/patch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/artifact/out/patch-javadoc-hadoop-common-project_hadoop-common-jdkPrivateBuild-11.0.15+10-Ubuntu-0ubuntu0.20.04.1.txt) | hadoop-common in the patch failed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1. |
| +1 :green_heart: | javadoc | 1m 59s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 1s | | the patch passed |
| +1 :green_heart: | shadedclient | 25m 37s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 8s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 13s | | The patch does not generate ASF License warnings. |
| | | 246m 22s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4255 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell |
| uname | Linux 0b98321b98d7 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 55b71c8b9bf2840f3bae05c97703eb2c08672b58 |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/testReport/ |
| Max. process+thread count | 2019 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918261848
##########
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 {
Review Comment:
Done. Weird only one other class in the package also extends it.
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918200036
##########
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();
Review Comment:
Ok, i'll then also had a beforeClass as its counter part. Yes there is no API to re-enable. Maybe, to limit access i can add a package protected enableSystemExit and enableSystemHalt so the test class can use them but not really everyone.
what do you think ?
--
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
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by 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
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918162069
##########
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();
Review Comment:
propose that this goes into an AfterClass method, which should also renenable system exits. Though I see there's no API to do that, so let's not worry about it
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -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) {
Review Comment:
refactor this into a private method as it is used enough times
##########
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 {
Review Comment:
`extends AbstractHadoopTestBase`
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918925836
##########
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>.
+ */
Review Comment:
done, also added a @return
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918241764
##########
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();
Review Comment:
I see that other tests in the util package use Before and After rather than BeforeClass and AfterClass. TestExitUtil does not rely on having resources that would benefit from being reserved only once for all the tests so i prefer to stick with the package's habit : i'll use before and after, not beforeClass and afterClass
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918261380
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -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) {
Review Comment:
done. I've made it so it handle the suppressor == suppressed scenario + can be used on the Error variable "caught" may be setted or be suppressed on the 2nd catch block in both exit and halt case.
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r917895399
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -159,138 +163,155 @@ 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 void terminate(ExitException ee)
throws ExitException {
final int status = ee.getExitCode();
- Error catched = null;
+ Error caught = null;
if (status != 0) {
try {
- //exit indicates a problem, log it
+ // 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
+ // 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 supressed
+ // all other kind of throwables are suppressed
ee.addSuppressed(t);
}
}
if (systemExitDisabled) {
try {
LOG.error("Terminate called", ee);
} catch (Error e) {
- if (catched == null) {
- catched = e; // errors will be re-thrown
+ // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
+ if (caught == null) {
+ caught = e;
} else {
- catched.addSuppressed(e); // 1st raised error has priority and will be re-thrown, so the 1st error supresses the 2nd
+ caught.addSuppressed(e);
}
} catch (Throwable t) {
- ee.addSuppressed(t); // all other kind of throwables are supressed
- }
- synchronized (ExitUtil.class) {
- if (!terminateCalled()) {
- firstExitException = ee;
- }
+ // all other kind of throwables are suppressed
+ ee.addSuppressed(t);
}
- if (catched != null) {
- catched.addSuppressed(ee);
- throw catched;
+ FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
+ if (caught != null) {
Review Comment:
Hi, sorry to have taken so long before an update.
I don't think we need such a check because the only things able to throw something would be either the JVM (which never reuse already thrown things) or the LOG object (which should never do that too). In my understanding of exception and any throwable in general is to never throw 2 times the same instance. If such a rule is respected, a suppressed can never a suppressor of itself through any suppressor/suppressed/rootCause chain of throwables. If for some reason the addSuppressed fails because of this, this more means there is something broken implemented elsewhere and this "elsewhere" should be fixed. If some piece of code reuse some already thrown throwables, even without failing, the suppress stuff could lead to memory leak (adding again and again new throwables to a singleton one kept and reuse somewhere).
In fact, if we really want to be safe, then we can forget the suppression stuff : this would keep code simple, no broken suppression error, no leak, at the cost of a slight loss of debug info when dumping throwable.
maybe we should rather go for this safe solution.
what do you think ?
--
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
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918827783
##########
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) {
Review Comment:
nice!
##########
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>.
+ */
Review Comment:
can you add some params to the javadoc to keep it happy
--
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
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r884819180
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -159,138 +163,155 @@ 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 void terminate(ExitException ee)
throws ExitException {
final int status = ee.getExitCode();
- Error catched = null;
+ Error caught = null;
if (status != 0) {
try {
- //exit indicates a problem, log it
+ // 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
+ // 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 supressed
+ // all other kind of throwables are suppressed
ee.addSuppressed(t);
}
}
if (systemExitDisabled) {
try {
LOG.error("Terminate called", ee);
} catch (Error e) {
- if (catched == null) {
- catched = e; // errors will be re-thrown
+ // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
+ if (caught == null) {
+ caught = e;
} else {
- catched.addSuppressed(e); // 1st raised error has priority and will be re-thrown, so the 1st error supresses the 2nd
+ caught.addSuppressed(e);
}
} catch (Throwable t) {
- ee.addSuppressed(t); // all other kind of throwables are supressed
- }
- synchronized (ExitUtil.class) {
- if (!terminateCalled()) {
- firstExitException = ee;
- }
+ // all other kind of throwables are suppressed
+ ee.addSuppressed(t);
}
- if (catched != null) {
- catched.addSuppressed(ee);
- throw catched;
+ FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
+ if (caught != null) {
Review Comment:
do you think we need a safety check to see if ee != caught before addSuppressed()? we've had problems with abfs in the past, where a cached exception was being rethrown in close()
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
+ assertNull(ExitUtil.getFirstExitException());
+
+ // simulate/check 1st call
+ try {
+ ExitUtil.terminate(ee1);
Review Comment:
use LambdaTestUtils.intercept() to make the call; and we can use assertSame for equivalenced
```
ExitException ex = intercept(ExitException.class, () ->
ExitUtil.terminate(ee1));
assertSame("ExitUtil.terminate should have rethrown its ExitException argument but it "
+"had thrown something else", ee1, ee);
```
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -159,138 +163,155 @@ 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;
Review Comment:
nit, add some spaces around !=
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
+ assertNull(ExitUtil.getFirstExitException());
+
+ // simulate/check 1st call
+ try {
+ ExitUtil.terminate(ee1);
+ fail("ExitUtil.terminate should have rethrown its ExitException argument but it returned");
+ } catch (ExitException ee) {
+ assertEquals("ExitUtil.terminate should have rethrown its ExitException argument but it "
+ +"had thrown something else", ee1, ee);
+ }
+ assertTrue(ExitUtil.terminateCalled());
+ assertEquals("ExitUtil.terminate should store its 1st call's ExitException",
Review Comment:
assertSame
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
Review Comment:
add messages here, so junit test failures woudl be meaningful.
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
+ assertNull(ExitUtil.getFirstExitException());
+
+ // simulate/check 1st call
+ try {
+ ExitUtil.terminate(ee1);
+ fail("ExitUtil.terminate should have rethrown its ExitException argument but it returned");
+ } catch (ExitException ee) {
+ assertEquals("ExitUtil.terminate should have rethrown its ExitException argument but it "
+ +"had thrown something else", ee1, ee);
+ }
+ assertTrue(ExitUtil.terminateCalled());
+ assertEquals("ExitUtil.terminate should store its 1st call's ExitException",
+ ee1, ExitUtil.getFirstExitException());
+
+ // simulate/check 2nd call not overwritting 1st one
+ try {
+ ExitUtil.terminate(ee2);
Review Comment:
intercept() and assertSame again
##########
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.junit.Assert.*;
+
+import org.apache.hadoop.util.ExitUtil.ExitException;
+import org.apache.hadoop.util.ExitUtil.HaltException;
+
+import org.junit.Test;
+
+public class TestExitUtil {
+
+ @Test
+ public void testGetSetExitExceptions() {
+ // 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());
+ assertNull(ExitUtil.getFirstExitException());
+
+ // simulate/check 1st call
+ try {
+ ExitUtil.terminate(ee1);
+ fail("ExitUtil.terminate should have rethrown its ExitException argument but it returned");
+ } catch (ExitException ee) {
+ assertEquals("ExitUtil.terminate should have rethrown its ExitException argument but it "
+ +"had thrown something else", ee1, ee);
+ }
+ assertTrue(ExitUtil.terminateCalled());
+ assertEquals("ExitUtil.terminate should store its 1st call's ExitException",
+ ee1, ExitUtil.getFirstExitException());
+
+ // simulate/check 2nd call not overwritting 1st one
+ try {
+ ExitUtil.terminate(ee2);
+ fail("ExitUtil.terminate should have rethrown its ExitException argument but it returned");
+ } catch (ExitException ee) {
+ assertEquals("ExitUtil.terminate should have rethrown its HaltException argument but it "
+ +"had thrown something else", ee2, ee);
+ }
+ assertTrue(ExitUtil.terminateCalled());
+ // 2nd call rethrown the 2nd ExitException yet only the 1st only should have been stored
+ assertEquals("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());
+ assertNull(ExitUtil.getFirstExitException());
+ } finally {
+ // cleanup
+ ExitUtil.resetFirstExitException();
+ }
+ }
+
+ @Test
+ public void testGetSetHaltExceptions() {
Review Comment:
same intercept and assertSame recommendations as the previous test
--
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
[GitHub] [hadoop] steveloughran commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1141140899
the inode stuff has been fixed elsewhere. if you rebase or merge your pr, it will go away. or we just ignore it, which simplifies keeping this commit history/discussion valid
--
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
[GitHub] [hadoop] steveloughran commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1183110785
+1, merging. thank you for your diligence here.
--
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
[GitHub] [hadoop] HerCath commented on a diff in pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
HerCath commented on code in PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#discussion_r918147667
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ExitUtil.java:
##########
@@ -159,138 +163,155 @@ 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 void terminate(ExitException ee)
throws ExitException {
final int status = ee.getExitCode();
- Error catched = null;
+ Error caught = null;
if (status != 0) {
try {
- //exit indicates a problem, log it
+ // 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
+ // 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 supressed
+ // all other kind of throwables are suppressed
ee.addSuppressed(t);
}
}
if (systemExitDisabled) {
try {
LOG.error("Terminate called", ee);
} catch (Error e) {
- if (catched == null) {
- catched = e; // errors will be re-thrown
+ // errors have higher priority again, if it's a 2nd error, the 1st one suprpesses it
+ if (caught == null) {
+ caught = e;
} else {
- catched.addSuppressed(e); // 1st raised error has priority and will be re-thrown, so the 1st error supresses the 2nd
+ caught.addSuppressed(e);
}
} catch (Throwable t) {
- ee.addSuppressed(t); // all other kind of throwables are supressed
- }
- synchronized (ExitUtil.class) {
- if (!terminateCalled()) {
- firstExitException = ee;
- }
+ // all other kind of throwables are suppressed
+ ee.addSuppressed(t);
}
- if (catched != null) {
- catched.addSuppressed(ee);
- throw catched;
+ FIRST_EXIT_EXCEPTION.compareAndSet(null, ee);
+ if (caught != null) {
Review Comment:
Finally I've done what you suggested since its efficient enough, robust enough, et does not clobber that much the code.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1180958336
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 54s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 37m 18s | | trunk passed |
| +1 :green_heart: | compile | 22m 56s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 20m 51s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 47s | | trunk passed |
| +1 :green_heart: | mvnsite | 2m 15s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 50s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 21s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 14s | | trunk passed |
| +1 :green_heart: | shadedclient | 23m 30s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 6s | | the patch passed |
| +1 :green_heart: | compile | 22m 14s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 22m 14s | | the patch passed |
| +1 :green_heart: | compile | 20m 40s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 20m 40s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 34s | | the patch passed |
| +1 :green_heart: | mvnsite | 2m 11s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 39s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 23s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 10s | | the patch passed |
| +1 :green_heart: | shadedclient | 23m 30s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 41s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 36s | | The patch does not generate ASF License warnings. |
| | | 215m 21s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/4/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4255 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 496b381aeb09 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 9e9dc762380d4f9c2a55c24412d3cbbdb19394bf |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/4/testReport/ |
| Max. process+thread count | 3152 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/4/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] hadoop-yetus commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1181989449
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 54s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 1 new or modified test files. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 40m 15s | | trunk passed |
| +1 :green_heart: | compile | 24m 55s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 21m 36s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 29s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 57s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 32s | | trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 4s | | trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 3s | | trunk passed |
| +1 :green_heart: | shadedclient | 25m 57s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 5s | | the patch passed |
| +1 :green_heart: | compile | 24m 2s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 24m 2s | | the patch passed |
| +1 :green_heart: | compile | 21m 37s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 21m 37s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 23s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 56s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 23s | | the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 1m 3s | | the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 3s | | the patch passed |
| +1 :green_heart: | shadedclient | 25m 51s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 18m 24s | | hadoop-common in the patch passed. |
| +1 :green_heart: | asflicense | 1m 17s | | The patch does not generate ASF License warnings. |
| | | 224m 32s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/5/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4255 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux d87a77336653 4.15.0-175-generic #184-Ubuntu SMP Thu Mar 24 17:48:36 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 518fefc06263d557206b21afe17ceb5338025371 |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/5/testReport/ |
| Max. process+thread count | 3135 (vs. ulimit of 5500) |
| modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4255/5/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
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
[GitHub] [hadoop] steveloughran commented on pull request #4255: HADOOP-18217. ExitUtil synchronized blocks reduced to avoid exit bloc…
Posted by GitBox <gi...@apache.org>.
steveloughran commented on PR #4255:
URL: https://github.com/apache/hadoop/pull/4255#issuecomment-1123496521
thanks for that detailed explanation.
you are right that spawning jvms and trying trigger timeouts would be trouble. lets just rely on code review here.
atomic references sound file
--
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