From 23a9829ec547bf50b336879370601a0100b5a742 Mon Sep 17 00:00:00 2001
From: Simone Bordet <simone.bordet@gmail.com>
Date: Mon, 23 Aug 2021 12:20:53 +0200
Subject: [PATCH] Fixes #6652 - Improve ReservedThreadExecutor dump.

Now adding/removing ReservedThread instances to _threads only in reservedWait().
In this way the window of time in which they can be caught for a dump() is shorter, producing more accurate dumps.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
---
 .../util/thread/ReservedThreadExecutor.java   | 83 +++++++++++--------
 1 file changed, 49 insertions(+), 34 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
index 924f1d0564ec..e4145d8da9bc 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
@@ -179,14 +179,19 @@ public void doStop() throws Exception
 
         super.doStop();
 
-        // Offer STOP task to all waiting reserved threads.
-        for (int i = _count.getAndSetLo(-1); i-- > 0;)
+        // Mark this instance as stopped.
+        int size = _count.getAndSetLo(-1);
+
+        // Offer the STOP task to all waiting reserved threads.
+        for (int i = 0; i < size; ++i)
         {
-            // yield to wait for any reserved threads that have incremented the size but not yet polled
+            // Yield to wait for any reserved threads that
+            // have incremented the size but not yet polled.
             Thread.yield();
             _queue.offer(STOP);
         }
-        // Interrupt any reserved thread missed the offer so it doesn't wait too long.
+        // Interrupt any reserved thread missed the offer,
+        // so they do not wait for the whole idle timeout.
         for (ReservedThread reserved : _threads)
         {
             Thread thread = reserved._thread;
@@ -251,7 +256,6 @@ private void startReservedThread()
             try
             {
                 ReservedThread thread = new ReservedThread();
-                _threads.add(thread);
                 _executor.execute(thread);
             }
             catch (Throwable e)
@@ -267,13 +271,13 @@ private void startReservedThread()
     public void dump(Appendable out, String indent) throws IOException
     {
         Dumpable.dumpObjects(out, indent, this,
-            new DumpableCollection("reserved", _threads));
+            new DumpableCollection("threads", _threads));
     }
 
     @Override
     public String toString()
     {
-        return String.format("%s@%x{s=%d/%d,p=%d}",
+        return String.format("%s@%x{reserved=%d/%d,pending=%d}",
             getClass().getSimpleName(),
             hashCode(),
             _count.getLo(),
@@ -301,38 +305,50 @@ private Runnable reservedWait()
             if (LOG.isDebugEnabled())
                 LOG.debug("{} waiting {}", this, ReservedThreadExecutor.this);
 
-            // Keep waiting until stopped, tasked or idle
-            while (_count.getLo() >= 0)
+            // This is now a reserved thread.
+            // Note that this thread must be added to the reserved set
+            // before checking for lo<0 (i.e. stopped), see doStop().
+            _threads.add(this);
+            try
             {
-                try
+                // Keep waiting until stopped, tasked or idle.
+                while (_count.getLo() >= 0)
                 {
-                    // Always poll at some period as safety to ensure we don't poll forever.
-                    Runnable task = _queue.poll(_idleTimeNanos, NANOSECONDS);
-                    if (LOG.isDebugEnabled())
-                        LOG.debug("{} task={} {}", this, task, ReservedThreadExecutor.this);
-                    if (task != null)
-                        return task;
-
-                    // we have idled out
-                    int size = _count.getLo();
-                    // decrement size if we have not also been stopped.
-                    while (size > 0)
+                    try
                     {
-                        if (_count.compareAndSetLo(size, --size))
-                            break;
-                        size = _count.getLo();
-                    }
-                    _state = size >= 0 ? State.IDLE : State.STOPPED;
-                    return STOP;
+                        // Always poll at some period as safety to ensure we don't poll forever.
+                        Runnable task = _queue.poll(_idleTimeNanos, NANOSECONDS);
+                        if (LOG.isDebugEnabled())
+                            LOG.debug("{} task={} {}", this, task, ReservedThreadExecutor.this);
+                        if (task != null)
+                            return task;
+
+                        // we have idled out
+                        int size = _count.getLo();
+                        // decrement size if we have not also been stopped.
+                        while (size > 0)
+                        {
+                            if (_count.compareAndSetLo(size, --size))
+                                break;
+                            size = _count.getLo();
+                        }
+                        _state = size >= 0 ? State.IDLE : State.STOPPED;
+                        return STOP;
 
+                    }
+                    catch (InterruptedException e)
+                    {
+                        LOG.ignore(e);
+                    }
                 }
-                catch (InterruptedException e)
-                {
-                    LOG.ignore(e);
-                }
+                _state = State.STOPPED;
+                return STOP;
+            }
+            finally
+            {
+                // No longer a reserved thread.
+                _threads.remove(this);
             }
-            _state = State.STOPPED;
-            return STOP;
         }
 
         @Override
@@ -405,7 +421,6 @@ public void run()
             {
                 if (LOG.isDebugEnabled())
                     LOG.debug("{} exited {}", this, ReservedThreadExecutor.this);
-                _threads.remove(this);
                 _thread = null;
             }
         }