diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java index ef59ff2a6f8..3941107d648 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ReservedThreadExecutor.java @@ -282,7 +282,7 @@ public class ReservedThreadExecutor extends AbstractLifeCycle implements TryExec { LOG.ignore(e); _size.getAndIncrement(); - _stack.addFirst(this); + _stack.offerFirst(this); return false; } } @@ -307,14 +307,13 @@ public class ReservedThreadExecutor extends AbstractLifeCycle implements TryExec if (task != null) return task; - // Because threads are held in a stack, excess threads will be - // idle. However, we cannot remove threads from the bottom of - // the stack, so we submit a poison pill job to stop the thread - // on top of the stack (which unfortunately will be the most - // recently used) - if (LOG.isDebugEnabled()) - LOG.debug("{} IDLE", this); - tryExecute(STOP); + if (_stack.remove(this)) + { + if (LOG.isDebugEnabled()) + LOG.debug("{} IDLE", this); + _size.decrementAndGet(); + return STOP; + } } catch (InterruptedException e) { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/ReservedThreadExecutorTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/ReservedThreadExecutorTest.java index f30f300a1ad..a68e5992388 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/ReservedThreadExecutorTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/ReservedThreadExecutorTest.java @@ -32,6 +32,8 @@ import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -183,6 +185,26 @@ public class ReservedThreadExecutorTest assertThat(_reservedExecutor.getAvailable(), is(0)); } + @Test + public void testReservedIdleTimeoutWithOneReservedThread() throws Exception + { + long idleTimeout = 500; + _reservedExecutor.stop(); + _reservedExecutor.setIdleTimeout(idleTimeout, TimeUnit.MILLISECONDS); + _reservedExecutor.start(); + + assertThat(_reservedExecutor.tryExecute(NOOP), is(false)); + Thread thread = _executor.startThread(); + assertNotNull(thread); + waitForAvailable(1); + + Thread.sleep(2 * idleTimeout); + + waitForAvailable(0); + thread.join(2 * idleTimeout); + assertFalse(thread.isAlive()); + } + protected void waitForAvailable(int size) throws InterruptedException { long started = System.nanoTime(); @@ -211,11 +233,16 @@ public class ReservedThreadExecutorTest _queue.addLast(task); } - public void startThread() + public Thread startThread() { Runnable task = _queue.pollFirst(); if (task != null) - new Thread(task).start(); + { + Thread thread = new Thread(task); + thread.start(); + return thread; + } + return null; } }