Fixes #4612 - ReservedThreadExecutor hangs when the last reserved thr…

…ead idles out. Explicitly removing the idled out thread from the stack, rather than calling tryExecute(). Side benefit is that we are now removing idled out threads that are least recently used. Signed-off-by: Simone Bordet <[email protected]>
jetty · Feb 27, 2020 · 75893da · 75893da
1 parent 65a22e5
commit 75893da
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 11 deletions.
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
@@ -282,7 +282,7 @@ public boolean offer(Runnable task)
             {
                 LOG.ignore(e);
                 _size.getAndIncrement();
-                _stack.addFirst(this);
+                _stack.offerFirst(this);
                 return false;
             }
         }
@@ -307,14 +307,13 @@ private Runnable reservedWait()
                     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
@@ -32,6 +32,8 @@
 
 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 void testShrink() throws Exception
         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 void execute(Runnable task)
             _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;
         }
     }