Fix InterruptedException silently dropping task in SmartExecutor.Limited.submit(Runnable)#2012
Conversation
…ted.submit(Runnable) Fixes #1993
| } | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Throwing a raw RuntimeException is an antipattern. Is there a subclass of RuntimeException we could throw instead?
Whatever exception we throw should be documented in javadoc for this method.
Is the exception handled upstream or is that something we also need to do?
There was a problem hiding this comment.
Pull request overview
Fixes a correctness issue in SmartExecutor.Limited.submit(Runnable) where an InterruptedException during semaphore.acquire() would restore the interrupt flag but silently drop the task (no execution and no signal to the caller). This change makes the failure visible to callers by throwing after restoring the interrupt status.
Changes:
- In
Limited.submit(Runnable), onInterruptedException, re-interrupt the thread and throw aRuntimeExceptionwrapping the original exception.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @throws RejectedExecutionException If this executor cannot accept the task. | ||
| */ | ||
| void submit(Runnable runnable); | ||
| void submit(Runnable runnable) throws RejectedExecutionException; |
There was a problem hiding this comment.
don't use throws clause for RuntimeExceptions, javadoc comment only
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Correct fix for a real concurrency bug. The previous code silently dropped the submitted task on InterruptedException; the fix properly restores the interrupt flag and throws RejectedExecutionException (wrapping the original cause), which is semantically precise and avoids violating the concurrency limit that Limited exists to enforce.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
|
@cstamas Please assign appropriate label to PR according to the type of change. |
Fixes #1993
In
Limited.submit(Runnable)(line 155), ifsemaphore.acquire()throwsInterruptedException, the interrupt flag was restored but the caller's task was never executed — it was silently dropped. Thesubmit(Callable)variant correctly handles this by returning a failedCompletableFuture.This fix throws a
RuntimeExceptionwrapping theInterruptedExceptionafter restoring the interrupt flag, so the caller is notified that the task could not be submitted.