[
https://issues.apache.org/jira/browse/LANG-1817?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ivan Ponomarev updated LANG-1817:
---------------------------------
Description:
h3. 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 observing {{Thread.currentThread().isInterrupted()}} after calling
{{UncheckedFuture.get()}} may incorrectly see {{{}false{}}}, even though an
interrupt was delivered.
For example,
{code:java}
class Worker implements Runnable {
private final UncheckedFuture<?> future;
Worker(UncheckedFuture<?> future) {
this.future = future;
}
@Override
public void run() {
while (!Thread.currentThread().isInterrupted()) {
// Block waiting for work or result
try {
future.get(); // <-- wrapped Future
doWork();
} catch (RuntimeException e) {
log.warn("Transient failure", e);
// continue loop
}
}
}
}
//Shutdown logic
executor.shutdownNow(); // interrupts worker threads
{code}
* Thread is interrupted.
* {{Future.get()}} throws {{{}InterruptedException{}}}.
* {{UncheckedFutureImpl.get()}} catches it and clears the {{{}interrupt{}}}.
It throws {{{}UncheckedInterruptedException{}}}.
* Caller catches or ignores the unchecked exception.
* Thread interrupt flag is now false.
* {{while (!Thread.currentThread().isInterrupted())}} continues running.
* {*}Even if the exception is not caught{*}, and the task returns a thread to
the pool as non-interrupted, then any higher-level cancellation logic that
relies on the flag won’t see it.
h3. Proposed fix
When wrapping {{{}InterruptedException{}}}, the interrupt status should be
preserved:
{code:java}
catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new UncheckedInterruptedException(e);
}
{code}
This matches standard Java concurrency best practices.
See Section 7.1 of "Java Concurrency in Practice" (Brian Goetz et al):
{quote}If [the task] is not simply going to propagate
{{InterruptedException}} to its caller, it should restore the interruption
status after
catching {{InterruptedException}} using {{Thread.currentThread().interrupt();}}
{quote}
See also
[code-review-checklists/java-concurrency/#IF.1|https://github.com/code-review-checklists/java-concurrency?tab=readme-ov-file#restore-interruption]
{quote}If some code propagates InterruptedException wrapped into another
exception (e. g. RuntimeException), is the interruption status of the current
thread restored before the wrapping exception is thrown?
{quote}
Proposed fix: https://github.com/apache/commons-lang/pull/1590
was:
h3. 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 observing {{Thread.currentThread().isInterrupted()}} after calling
{{UncheckedFuture.get()}} may incorrectly see {{{}false{}}}, even though an
interrupt was delivered.
For example,
{code:java}
class Worker implements Runnable {
private final UncheckedFuture<?> future;
Worker(UncheckedFuture<?> future) {
this.future = future;
}
@Override
public void run() {
while (!Thread.currentThread().isInterrupted()) {
// Block waiting for work or result
try {
future.get(); // <-- wrapped Future
doWork();
} catch (RuntimeException e) {
log.warn("Transient failure", e);
// continue loop
}
}
}
}
//Shutdown logic
executor.shutdownNow(); // interrupts worker threads
{code}
* Thread is interrupted.
* {{Future.get()}} throws {{{}InterruptedException{}}}.
* {{UncheckedFutureImpl.get()}} catches it and clears the {{{}interrupt{}}}.
It throws {{{}UncheckedInterruptedException{}}}.
* Caller catches or ignores the unchecked exception.
* Thread interrupt flag is now false.
* {{while (!Thread.currentThread().isInterrupted())}} continues running.
* {*}Even if the exception is not caught{*}, and the task returns a thread to
the pool as non-interrupted, then any higher-level cancellation logic that
relies on the flag won’t see it.
h3. Proposed fix
When wrapping {{{}InterruptedException{}}}, the interrupt status should be
preserved:
{code:java}
catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new UncheckedInterruptedException(e);
}
{code}
This matches standard Java concurrency best practices.
See Section 7.1 of "Java Concurrency in Practice" (Brian Goetz et al):
{quote}If [the task] is not simply going to propagate
{{InterruptedException}} to its caller, it should restore the interruption
status after
catching {{InterruptedException}} using {{Thread.currentThread().interrupt();}}
{quote}
See also
[code-review-checklists/java-concurrency/#IF.1|https://github.com/code-review-checklists/java-concurrency?tab=readme-ov-file#restore-interruption]
{quote}If some code propagates InterruptedException wrapped into another
exception (e. g. RuntimeException), is the interruption status of the current
thread restored before the wrapping exception is thrown?
{quote}
> UncheckedFutureImpl clears thread interrupt status when wrapping
> InterruptedException
> -------------------------------------------------------------------------------------
>
> Key: LANG-1817
> URL: https://issues.apache.org/jira/browse/LANG-1817
> Project: Commons Lang
> Issue Type: Bug
> Reporter: Ivan Ponomarev
> Priority: Major
>
> h3. 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 observing {{Thread.currentThread().isInterrupted()}} after calling
> {{UncheckedFuture.get()}} may incorrectly see {{{}false{}}}, even though an
> interrupt was delivered.
> For example,
> {code:java}
> class Worker implements Runnable {
> private final UncheckedFuture<?> future;
> Worker(UncheckedFuture<?> future) {
> this.future = future;
> }
> @Override
> public void run() {
> while (!Thread.currentThread().isInterrupted()) {
> // Block waiting for work or result
> try {
> future.get(); // <-- wrapped Future
> doWork();
> } catch (RuntimeException e) {
> log.warn("Transient failure", e);
> // continue loop
> }
> }
> }
> }
> //Shutdown logic
> executor.shutdownNow(); // interrupts worker threads
> {code}
> * Thread is interrupted.
> * {{Future.get()}} throws {{{}InterruptedException{}}}.
> * {{UncheckedFutureImpl.get()}} catches it and clears the {{{}interrupt{}}}.
> It throws {{{}UncheckedInterruptedException{}}}.
> * Caller catches or ignores the unchecked exception.
> * Thread interrupt flag is now false.
> * {{while (!Thread.currentThread().isInterrupted())}} continues running.
> * {*}Even if the exception is not caught{*}, and the task returns a thread
> to the pool as non-interrupted, then any higher-level cancellation logic that
> relies on the flag won’t see it.
> h3. Proposed fix
> When wrapping {{{}InterruptedException{}}}, the interrupt status should be
> preserved:
> {code:java}
> catch (InterruptedException e) {
> Thread.currentThread().interrupt();
> throw new UncheckedInterruptedException(e);
> }
> {code}
>
> This matches standard Java concurrency best practices.
> See Section 7.1 of "Java Concurrency in Practice" (Brian Goetz et al):
> {quote}If [the task] is not simply going to propagate
> {{InterruptedException}} to its caller, it should restore the interruption
> status after
> catching {{InterruptedException}} using
> {{Thread.currentThread().interrupt();}}
> {quote}
> See also
> [code-review-checklists/java-concurrency/#IF.1|https://github.com/code-review-checklists/java-concurrency?tab=readme-ov-file#restore-interruption]
> {quote}If some code propagates InterruptedException wrapped into another
> exception (e. g. RuntimeException), is the interruption status of the current
> thread restored before the wrapping exception is thrown?
> {quote}
> Proposed fix: https://github.com/apache/commons-lang/pull/1590
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
