Skip to content

Commit 6dad156

Browse files
committed
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 a40647a commit 6dad156

File tree

2 files changed

+69
-1
lines changed

2 files changed

+69
-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: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.core.task;
1818

19+
import java.time.Duration;
20+
1921
import org.junit.jupiter.api.Test;
2022

2123
import org.springframework.util.ConcurrencyThrottleSupport;
@@ -24,6 +26,12 @@
2426
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2527
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
2628
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
29+
import static org.assertj.core.api.Assertions.fail;
30+
import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
31+
import static org.mockito.ArgumentMatchers.any;
32+
import static org.mockito.BDDMockito.willDoNothing;
33+
import static org.mockito.Mockito.doThrow;
34+
import static org.mockito.Mockito.spy;
2735

2836
/**
2937
* @author Rick Evans
@@ -69,6 +77,58 @@ void taskRejectedWhenConcurrencyLimitReached() {
6977
}
7078
}
7179

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

0 commit comments

Comments
 (0)