LANG-1817: UncheckedFutureImpl clears thread interrupt status when wrapping InterruptedException

[inponomarev]

Problem

UncheckedFutureImpl.get() and get(long, TimeUnit) catch InterruptedException and rethrow it as UncheckedInterruptedException, but do not restore the thread’s interrupt status.

Catching InterruptedException clears the interrupt flag. As a result, callers relying on Thread.currentThread().isInterrupted() could incorrectly observe a non-interrupted state after calling UncheckedFuture.get(), breaking standard cooperative cancellation patterns.

This issue is tracked as LANG-1817
.

Changes

Changes

Restore the interrupt status before rethrowing UncheckedInterruptedException:

catch (InterruptedException e) {
    Thread.currentThread().interrupt();
    throw new UncheckedInterruptedException(e);
}

This is applied to both get() overloads.

Added regression tests that:

• execute UncheckedFuture.get() and get(timeout, unit) on a worker thread,
• interrupt that thread via ExecutorService.shutdownNow(),
• assert that the interrupt flag remains set after the unchecked exception is thrown.

[garydgregory]

Hello @inponomarev

Thank you for the PR.

There are a few places where we catch InterrupterdException, so we should be consistent on how this is handled. I also wonder if we should // comment each time to clarify each use case:

• ThreadUtils.sleepQuietly(Duration)
• BackgroundInitializer.get()
• BackgroundInitializer.isInitialized()
• UncheckedFutureImpl.get()
• UncheckedFutureImpl.get(long, TimeUnit)

WDYT?

[inponomarev]

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.

[garydgregory]

Hello @inponomarev

Yes, a consistent approach throughout would be good IMO. And not just in Apache Commons Lang, but in all of Commons, so let's start here. This is an easily overlooked and obviously poorly or incorrectly tested code path. TY!

FWIW, one item Commons Components (Lang, IO, and maybe others) have been vague about is whether their "quiet" semantics apply only to checked or both checked and unchecked exceptions. InterrupterdException is a special case as you note.

I should add that for now, the quiet methods should remain so (not throw), or this will likely break apps that close a bunch of resources in consecutive calls. Restoring the interrupt flag should obviously be done though.

[garydgregory]

Hello @inponomarev
FYI:

Error:  Errors: 
Error:    UncheckedFutureTest.interruptFlagIsPreservedOnGetWithTimeout:129->assertInterruptPreserved:155 » Execution org.opentest4j.AssertionFailedError: expected: <true> but was: <false>

[inponomarev]

OK please give me a couple of days for this one, probably I have to re-write the tests for deterministic behaviour

meanwhile could you have a look at #1587 -- I fixed your review comments, this one should be easier

Thanks for your fast replies!