From 85611e5df0d1d4ba2f5c59f5ac7f77724634384f Mon Sep 17 00:00:00 2001
From: Simone Bordet <simone.bordet@gmail.com>
Date: Tue, 24 Aug 2021 10:57:14 +0200
Subject: [PATCH] Fixes #6652 - Improve ReservedThreadExecutor dump.

Incorporate suggestions from review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
---
 .../util/thread/ReservedThreadExecutor.java   | 32 +++++++++++--------
 .../thread/ReservedThreadExecutorTest.java    |  3 --
 2 files changed, 18 insertions(+), 17 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 d71a357199ed..f861470cfc4c 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
@@ -19,7 +19,6 @@
 package org.eclipse.jetty.util.thread;
 
 import java.io.IOException;
-import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
@@ -35,6 +34,7 @@
 import org.eclipse.jetty.util.annotation.ManagedObject;
 import org.eclipse.jetty.util.component.AbstractLifeCycle;
 import org.eclipse.jetty.util.component.Dumpable;
+import org.eclipse.jetty.util.component.DumpableCollection;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -43,14 +43,14 @@
 import static org.eclipse.jetty.util.AtomicBiInteger.getLo;
 
 /**
- * An Executor using pre-allocated/reserved Threads from a wrapped Executor.
- * <p>Calls to {@link #execute(Runnable)} on a {@link ReservedThreadExecutor} will either succeed
- * with a Thread immediately being assigned the Runnable task, or fail if no Thread is
- * available.
- * <p>Threads are reserved lazily, with a new reserved threads being allocated from the
- * {@link Executor} passed to the constructor.  Whenever 1 or more reserved threads have been
+ * <p>A TryExecutor using pre-allocated/reserved threads from an external Executor.</p>
+ * <p>Calls to {@link #tryExecute(Runnable)} on ReservedThreadExecutor will either
+ * succeed with a reserved thread immediately being assigned the task, or fail if
+ * no reserved thread is available.</p>
+ * <p>Threads are reserved lazily, with new reserved threads being allocated from the external
+ * {@link Executor} passed to the constructor. Whenever 1 or more reserved threads have been
  * idle for more than {@link #getIdleTimeoutMs()} then one reserved thread will return to
- * the executor.
+ * the external Executor.</p>
  */
 @ManagedObject("A pool for reserved threads")
 public class ReservedThreadExecutor extends AbstractLifeCycle implements TryExecutor, Dumpable
@@ -67,7 +67,7 @@ public void run()
         @Override
         public String toString()
         {
-            return "STOP!";
+            return "STOP";
         }
     };
 
@@ -77,7 +77,6 @@ public String toString()
     private final SynchronousQueue<Runnable> _queue = new SynchronousQueue<>(false);
     private final AtomicBiInteger _count = new AtomicBiInteger(); // hi=pending; lo=size;
     private final AtomicLong _lastEmptyTime = new AtomicLong(System.nanoTime());
-
     private ThreadPoolBudget.Lease _lease;
     private long _idleTimeNanos = DEFAULT_IDLE_TIMEOUT;
 
@@ -194,7 +193,10 @@ public void doStop() throws Exception
 
         // Interrupt any reserved thread missed the offer,
         // so they do not wait for the whole idle timeout.
-        _threads.stream().filter(ReservedThread::isReserved).map(t -> t._thread).forEach(Thread::interrupt);
+        _threads.stream()
+            .filter(ReservedThread::isReserved)
+            .map(t -> t._thread)
+            .forEach(Thread::interrupt);
         _threads.clear();
         _count.getAndSetHi(0);
     }
@@ -269,7 +271,10 @@ private void startReservedThread()
     public void dump(Appendable out, String indent) throws IOException
     {
         Dumpable.dumpObjects(out, indent, this,
-            _threads.stream().filter(ReservedThread::isReserved).collect(Collectors.toList()));
+            new DumpableCollection("threads",
+                _threads.stream()
+                    .filter(ReservedThread::isReserved)
+                    .collect(Collectors.toList())));
     }
 
     @Override
@@ -300,7 +305,7 @@ private class ReservedThread implements Runnable
 
         private boolean isReserved()
         {
-            return Objects.equals(_state, State.RESERVED);
+            return _state == State.RESERVED;
         }
 
         private Runnable reservedWait()
@@ -331,7 +336,6 @@ private Runnable reservedWait()
                     }
                     _state = size >= 0 ? State.IDLE : State.STOPPED;
                     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 f15b707047c7..bead74b6ed23 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
@@ -360,13 +360,10 @@ public void run()
         while (!reserved.tryExecute(() -> {}))
             Thread.yield();
 
-        System.err.println(reserved.dump());
-
         reserved.stop();
         pool.stop();
 
         assertThat(usedReserved.get(), greaterThan(0));
         assertThat(usedReserved.get() + usedPool.get(), is(LOOPS));
-        // System.err.printf("reserved=%d pool=%d total=%d%n", usedReserved.get(), usedPool.get(), LOOPS);
     }
 }