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

[HerCath]

…king halt + enlarged catches (robustness) to all Throwables (not just Exceptions)

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:

• 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?
• If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

[steveloughran]

looks good; but the variable will need to be called `caught"

how to test this?

[steveloughran]

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

[HerCath]

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 their own machine.

[steveloughran]

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

[HerCath]

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 :/)

[steveloughran]

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

[steveloughran]

happy with the production code, now it's test tuning

[ashutoshcipher]

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.

[steveloughran]

this has now got big...propose some refactoring of the common setter.

[steveloughran]

LGTM. i would have expected javadoc to complain about undocumented params; maybe it is different on private methods. anyway, add those and the patch is ready to go

+1 pending the javadoc tweak

[hadoop-yetus]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 54s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s		The patch appears to include 1 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	40m 15s		trunk passed
+1 💚	compile	24m 55s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	21m 36s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 29s		trunk passed
+1 💚	mvnsite	1m 57s		trunk passed
+1 💚	javadoc	1m 32s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 4s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 3s		trunk passed
+1 💚	shadedclient	25m 57s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 5s		the patch passed
+1 💚	compile	24m 2s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	24m 2s		the patch passed
+1 💚	compile	21m 37s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	21m 37s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 23s		the patch passed
+1 💚	mvnsite	1m 56s		the patch passed
+1 💚	javadoc	1m 23s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 3s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 3s		the patch passed
+1 💚	shadedclient	25m 51s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	18m 24s		hadoop-common in the patch passed.
+1 💚	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	#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 / 518fefc
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.

[steveloughran]

+1, merging. thank you for your diligence here.

[steveloughran]

+cherrypicked to branch-3.3; ran new test suite and all is good.