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

[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