inponomarev commented on PR #1590: URL: https://github.com/apache/commons-lang/pull/1590#issuecomment-3808797163
Thank you for the review and for pointing out the other places where `InterruptedException` is handled inconsistently! My view here is aligned with the Brian Goetz "Java Concurrency in Practice" book: swallowing `InterruptedException` without restoring the interrupt flag is prohibited. I also summarise this approach in one of my lectures (slide linked below), which reflects the same reasoning: https://inponomarev.github.io/java-mipt/slides11/index-en.html#/_what_should_we_do_with_interruptedexception For the cases you mentioned: * `ThreadUtils.sleepQuietly(Duration)` This one is particularly questionable. I understand why such a utility exists: InterruptedException is annoying in single-threaded or “fire-and-forget” code. But if this ever runs in a pooled worker thread, swallowing the interrupt can cause very unpleasant shutdown behaviour. I would strongly recommend restoring the interrupt status and wrapping it into an unchecked exception in the catch block. * `BackgroundInitializer.get() / isInitialized()` It looks like interruption is handled correctly in one place and incorrectly in another. Adding a missing `Thread.currentThread().interrupt()` would be enough to make this consistent. * `UncheckedFutureImpl` This is the subject of this PR If you’d like, I’m happy to extend this PR and harmonise all of these cases so they follow the same pattern. -- 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]
