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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
