Skip to content

Commit 14579b7

Browse files
NYgometsjhoeller
authored andcommitted
Fix concurrency permit leak causing permanent deadlock in SimpleAsyncTaskExecutor
When concurrency limiting is enabled via setConcurrencyLimit() and thread creation fails in doExecute() (e.g., OutOfMemoryError from Thread.start()), the concurrency permit acquired by beforeAccess() is never released because TaskTrackingRunnable.run() never executes. This causes the concurrency count to permanently remain at the limit, causing all subsequent task submissions to block forever in ConcurrencyThrottleSupport.onLimitReached(). Root cause: - beforeAccess() increments concurrencyCount - doExecute() throws Error before thread starts - TaskTrackingRunnable.run() never executes - afterAccess() in finally block never called - Concurrency permit permanently leaked Solution: Wrap doExecute() in try-catch block in the concurrency throttle path and call afterAccess() in catch block to ensure permit is always released, even when thread creation fails. The fix only applies to the concurrency throttle path. The activeThreads-only path does not need fixing because it never calls beforeAccess(), so there is no permit to leak. Test approach: The test simulates thread creation failure and verifies that a subsequent execution does not deadlock. The first execution should fail with some exception (type doesn't matter), and the second execution should complete within timeout if the permit was properly released. Signed-off-by: Park Juhyeong <wngud5957@naver.com>
1 parent 5471961 commit 14579b7

File tree

2 files changed

+71
-1
lines changed

2 files changed

+71
-1
lines changed

spring-core/src/main/java/org/springframework/core/task/SimpleAsyncTaskExecutor.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,15 @@ public void execute(Runnable task, long startTimeout) {
313313
Runnable taskToUse = (this.taskDecorator != null ? this.taskDecorator.decorate(task) : task);
314314
if (isThrottleActive() && startTimeout > TIMEOUT_IMMEDIATE) {
315315
this.concurrencyThrottle.beforeAccess();
316-
doExecute(new TaskTrackingRunnable(taskToUse));
316+
try {
317+
doExecute(new TaskTrackingRunnable(taskToUse));
318+
}
319+
catch (Throwable ex) {
320+
// Release concurrency permit if thread creation fails
321+
this.concurrencyThrottle.afterAccess();
322+
throw new TaskRejectedException(
323+
"Failed to start execution thread for task: " + task, ex);
324+
}
317325
}
318326
else if (this.activeThreads != null) {
319327
doExecute(new TaskTrackingRunnable(taskToUse));

spring-core/src/test/java/org/springframework/core/task/SimpleAsyncTaskExecutorTests.java

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616

1717
package org.springframework.core.task;
1818

19+
import java.util.concurrent.CountDownLatch;
20+
import java.util.concurrent.TimeUnit;
21+
1922
import org.junit.jupiter.api.Test;
2023

2124
import org.springframework.util.ConcurrencyThrottleSupport;
@@ -24,6 +27,12 @@
2427
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2528
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
2629
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
30+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
31+
import static org.mockito.ArgumentMatchers.any;
32+
import static org.mockito.BDDMockito.willCallRealMethod;
33+
import static org.mockito.Mockito.doThrow;
34+
import static org.mockito.Mockito.spy;
35+
2736

2837
/**
2938
* @author Rick Evans
@@ -69,6 +78,59 @@ void taskRejectedWhenConcurrencyLimitReached() {
6978
}
7079
}
7180

81+
/**
82+
* Verify that when thread creation fails in doExecute() while concurrency
83+
* limiting is active, the concurrency permit is properly released to
84+
* prevent permanent deadlock.
85+
*
86+
* <p>This test reproduces a critical bug where OutOfMemoryError from
87+
* Thread.start() causes the executor to permanently deadlock:
88+
* <ol>
89+
* <li>beforeAccess() increments concurrencyCount
90+
* <li>doExecute() throws Error before thread starts
91+
* <li>TaskTrackingRunnable.run() never executes
92+
* <li>afterAccess() in finally block never called
93+
* <li>Subsequent tasks block forever in onLimitReached()
94+
* </ol>
95+
*
96+
* <p>Test approach: The first execute() should fail with some exception
97+
* (type doesn't matter - could be Error or TaskRejectedException).
98+
* The second execute() is the real test: it should complete without
99+
* deadlock if the permit was properly released.
100+
*/
101+
@Test
102+
void executeFailsToStartThreadReleasesConcurrencyPermit() throws InterruptedException {
103+
// Arrange
104+
SimpleAsyncTaskExecutor executor = spy(new SimpleAsyncTaskExecutor());
105+
executor.setConcurrencyLimit(1); // Enable concurrency limiting
106+
107+
Runnable task = () -> {};
108+
Error failure = new OutOfMemoryError("TEST: Cannot start thread");
109+
110+
// Simulate thread creation failure
111+
doThrow(failure).when(executor).doExecute(any(Runnable.class));
112+
113+
// Act - First execution fails
114+
// Both "before fix" (throws Error) and "after fix" (throws TaskRejectedException)
115+
// should throw some exception here - that's expected and correct
116+
assertThatThrownBy(() -> executor.execute(task))
117+
.isInstanceOf(Throwable.class);
118+
119+
// Arrange - Reset mock to allow second execution to succeed
120+
willCallRealMethod().given(executor).doExecute(any(Runnable.class));
121+
122+
// Assert - Second execution should NOT deadlock
123+
// This is the real test: if permit was leaked, this will timeout
124+
CountDownLatch latch = new CountDownLatch(1);
125+
executor.execute(() -> latch.countDown());
126+
127+
boolean completed = latch.await(1, TimeUnit.SECONDS);
128+
129+
assertThat(completed)
130+
.withFailMessage("Executor should not deadlock if concurrency permit was properly released after first failure")
131+
.isTrue();
132+
}
133+
72134
@Test
73135
void threadNameGetsSetCorrectly() {
74136
String customPrefix = "chankPop#";

0 commit comments

Comments
 (0)