Fixes #11098 - Sporadic NPE in ArrayByteBufferPool.evict(). (#11204)

* Reorganized ArrayByteBufferPool.evict() code to avoid NPE.
* Fixed acquire logic to take into account that a reserved entry may fail to be enabled because it may be concurrently removed.
* Small change to ConcurrentPool: no need to null check the final ConcurrentEntry.holder field.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java

@@ -207,23 +207,23 @@ public RetainableByteBuffer acquire(int size, boolean direct)
                     if (!reservedEntry.release())
                         reservedEntry.remove();
                 });
-                reservedEntry.enable(buffer, true);
-                if (direct)
-                    _currentDirectMemory.addAndGet(buffer.capacity());
-                else
-                    _currentHeapMemory.addAndGet(buffer.capacity());
-                releaseExcessMemory(direct);
-            }
-            else
-            {
-                buffer = newRetainableByteBuffer(size, direct, this::removed);
+
+                // A reserved entry may become old and be evicted before it is enabled.
+                if (reservedEntry.enable(buffer, true))
+                {
+                    if (direct)
+                        _currentDirectMemory.addAndGet(buffer.capacity());
+                    else
+                        _currentHeapMemory.addAndGet(buffer.capacity());
+                    releaseExcessMemory(direct);
+                    return buffer;
+                }
             }
+            return newRetainableByteBuffer(size, direct, this::removed);
         }
-        else
-        {
-            buffer = entry.getPooled();
-            ((Buffer)buffer).acquire();
-        }
+
+        buffer = entry.getPooled();
+        ((Buffer)buffer).acquire();
         return buffer;
     }
 
@@ -409,18 +409,27 @@ private void evict(boolean direct, long excess)
                 if (oldestEntry == null)
                     continue;
 
-                if (oldestEntry.remove())
-                {
-                    RetainableByteBuffer buffer = oldestEntry.getPooled();
-                    int clearedCapacity = buffer.capacity();
-                    if (direct)
-                        _currentDirectMemory.addAndGet(-clearedCapacity);
-                    else
-                        _currentHeapMemory.addAndGet(-clearedCapacity);
-                    totalClearedCapacity += clearedCapacity;
-                    removed(buffer);
-                }
-                // else a concurrent thread evicted the same entry -> do not account for its capacity.
+                // Get the pooled buffer now in case we can evict below.
+                // The buffer may be null if the entry has been reserved but
+                // not yet enabled, or the entry has been removed by a concurrent
+                // thread, that may also have nulled-out the pooled buffer.
+                RetainableByteBuffer buffer = oldestEntry.getPooled();
+                if (buffer == null)
+                    continue;
+
+                // A concurrent thread evicted the same entry.
+                // A successful Entry.remove() call may null-out the pooled buffer.
+                if (!oldestEntry.remove())
+                    continue;
+
+                // We can evict, clear the buffer capacity.
+                int clearedCapacity = buffer.capacity();
+                if (direct)
+                    _currentDirectMemory.addAndGet(-clearedCapacity);
+                else
+                    _currentHeapMemory.addAndGet(-clearedCapacity);
+                totalClearedCapacity += clearedCapacity;
+                removed(buffer);
             }
         }
 

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ConcurrentPool.java

@@ -282,7 +282,7 @@ private boolean remove(Entry<P> entry)
         // No need to lock, no race with reserve()
         // and the race with terminate() is harmless.
         Holder<P> holder = ((ConcurrentEntry<P>)entry).getHolder();
-        boolean evicted = holder != null && entries.remove(holder);
+        boolean evicted = entries.remove(holder);
         if (LOG.isDebugEnabled())
             LOG.debug("evicted {} {} for {}", evicted, entry, this);
 
@@ -418,16 +418,16 @@ public static class ConcurrentEntry<E> implements Entry<E>
         //    1+ -> multiplex count
         private final AtomicBiInteger state = new AtomicBiInteger(0, -1);
         private final ConcurrentPool<E> pool;
+        private final Holder<E> holder;
         // The pooled object. This is not volatile as it is set once and then never changed.
         // Other threads accessing must check the state field above first, so a good before/after
         // relationship exists to make a memory barrier.
         private E pooled;
-        private final Holder<E> holder;
 
         public ConcurrentEntry(ConcurrentPool<E> pool)
         {
             this.pool = pool;
-            holder = new Holder<>(this);
+            this.holder = new Holder<>(this);
         }
 
         private Holder<E> getHolder()