From 115ee1cf3968d1027832852ed54304fffe60a77b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 20 Sep 2024 16:50:39 +0300 Subject: [PATCH] Fixes #12289 - Improve ConcurrentPool concurrency. (#12290) A call to `sweep()`, although protected by the lock for concurrent calls to `reserve()`, may be concurrent with `remove(Entry)`. `remove(Entry)` in turn calls `entries.remove(Object)`, so that the concurrent iteration in `sweep()` over `entries` fails with an `ArrayIndexOutOfBoundsException`. Now using the bulk `entries.removeIf(Predicate)` method in `sweep()`, so that sweeping is atomic with respect to `entries.remove(Object)`. Fixed other occurrences of manual iteration over CopyOnWriteArrayList that may be concurrent with removals. Signed-off-by: Simone Bordet --- .../jetty/server/handler/ContextHandler.java | 4 +-- .../eclipse/jetty/util/ConcurrentPool.java | 27 ++++++++----------- .../java/org/eclipse/jetty/util/TypeUtil.java | 15 +++++++++++ .../ee10/servlet/ServletContextHandler.java | 16 +++++------ .../jetty/ee10/servlet/SessionHandler.java | 5 ++-- .../jetty/ee9/nested/ContextHandler.java | 12 ++++----- .../jetty/ee9/nested/SessionHandler.java | 5 ++-- 7 files changed, 48 insertions(+), 36 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java index 08f0c5047aad..54dcf4235706 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java @@ -653,11 +653,11 @@ protected void exitScope(Request request, Context lastContext, ClassLoader lastL */ protected void notifyExitScope(Request request) { - for (int i = _contextListeners.size(); i-- > 0; ) + for (ContextScopeListener listener : TypeUtil.reverse(_contextListeners)) { try { - _contextListeners.get(i).exitScope(_context, request); + listener.exitScope(_context, request); } catch (Throwable e) { diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ConcurrentPool.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ConcurrentPool.java index c96a75f6c236..0e9c1309c594 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ConcurrentPool.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/ConcurrentPool.java @@ -87,7 +87,7 @@ public ConcurrentPool(StrategyType strategyType, int maxSize) * @param cache whether a {@link ThreadLocal} cache should be used for the most recently released entry * @deprecated cache is no longer supported. Use {@link StrategyType#THREAD_ID} */ - @Deprecated + @Deprecated(since = "12.0.4", forRemoval = true) public ConcurrentPool(StrategyType strategyType, int maxSize, boolean cache) { this(strategyType, maxSize, pooled -> 1); @@ -103,7 +103,7 @@ public ConcurrentPool(StrategyType strategyType, int maxSize, boolean cache) * @param maxMultiplex a function that given the pooled object returns the max multiplex count * @deprecated cache is no longer supported. Use {@link StrategyType#THREAD_ID} */ - @Deprecated + @Deprecated(since = "12.0.4", forRemoval = true) public ConcurrentPool(StrategyType strategyType, int maxSize, boolean cache, ToIntFunction

maxMultiplex) { this(strategyType, maxSize, maxMultiplex); @@ -148,7 +148,7 @@ private void leaked(Holder

holder) { leaked.increment(); if (LOG.isDebugEnabled()) - LOG.debug("Leaked " + holder); + LOG.debug("Leaked {}", holder); leaked(); } @@ -195,15 +195,14 @@ public Entry

reserve() void sweep() { - for (int i = 0; i < entries.size(); i++) + // Remove entries atomically with respect to remove(Entry). + entries.removeIf(holder -> { - Holder

holder = entries.get(i); - if (holder.getEntry() == null) - { - entries.remove(i--); + boolean remove = holder.getEntry() == null; + if (remove) leaked(holder); - } - } + return remove; + }); } @Override @@ -285,8 +284,7 @@ private boolean remove(Entry

entry) if (!removed) return false; - // No need to lock, no race with reserve() - // and the race with terminate() is harmless. + // In a harmless race with reserve()/sweep()/terminate(). Holder

holder = ((ConcurrentEntry

)entry).getHolder(); boolean evicted = entries.remove(holder); if (LOG.isDebugEnabled()) @@ -313,10 +311,7 @@ public Collection> terminate() // Field this.terminated must be modified with the lock held // because the list of entries is modified, see reserve(). terminated = true; - copy = entries.stream() - .map(Holder::getEntry) - .filter(Objects::nonNull) - .toList(); + copy = stream().toList(); entries.clear(); } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java index 7c21e92731d1..d1658fdafd33 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/TypeUtil.java @@ -173,6 +173,21 @@ public static List asList(T[] a) return Arrays.asList(a); } + /** + *

Returns a new list with the elements of the specified list in reverse order.

+ *

The specified list is not modified, differently from {@link Collections#reverse(List)}.

+ * + * @param list the list whose elements are to be reversed + * @return a new list with the elements in reverse order + * @param the element type + */ + public static List reverse(List list) + { + List result = new ArrayList<>(list); + Collections.reverse(result); + return result; + } + /** * Class from a canonical name for a type. * diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index c7f07432d536..eb6e4e081135 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -95,6 +95,7 @@ import org.eclipse.jetty.util.ExceptionUtil; import org.eclipse.jetty.util.Loader; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.annotation.ManagedAttribute; import org.eclipse.jetty.util.annotation.ManagedObject; @@ -518,8 +519,7 @@ public void contextDestroyed() throws Exception //Call context listeners Throwable multiException = null; ServletContextEvent event = new ServletContextEvent(getServletContext()); - Collections.reverse(_destroyServletContextListeners); - for (ServletContextListener listener : _destroyServletContextListeners) + for (ServletContextListener listener : TypeUtil.reverse(_destroyServletContextListeners)) { try { @@ -574,17 +574,17 @@ protected void requestDestroyed(Request baseRequest, HttpServletRequest request) if (!_servletRequestListeners.isEmpty()) { final ServletRequestEvent sre = new ServletRequestEvent(getServletContext(), request); - for (int i = _servletRequestListeners.size(); i-- > 0; ) + for (ServletRequestListener listener : TypeUtil.reverse(_servletRequestListeners)) { - _servletRequestListeners.get(i).requestDestroyed(sre); + listener.requestDestroyed(sre); } } if (!_servletRequestAttributeListeners.isEmpty()) { - for (int i = _servletRequestAttributeListeners.size(); i-- > 0; ) + for (ServletRequestAttributeListener listener : TypeUtil.reverse(_servletRequestAttributeListeners)) { - scopedRequest.removeEventListener(_servletRequestAttributeListeners.get(i)); + scopedRequest.removeEventListener(listener); } } } @@ -1223,11 +1223,11 @@ protected void notifyExitScope(Request request) ServletContextRequest scopedRequest = Request.as(request, ServletContextRequest.class); if (!_contextListeners.isEmpty()) { - for (int i = _contextListeners.size(); i-- > 0; ) + for (ServletContextScopeListener listener : TypeUtil.reverse(_contextListeners)) { try { - _contextListeners.get(i).exitScope(getContext(), scopedRequest); + listener.exitScope(getContext(), scopedRequest); } catch (Throwable e) { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java index 2331e48e9c9f..50bfa347b7fd 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java @@ -47,6 +47,7 @@ import org.eclipse.jetty.session.ManagedSession; import org.eclipse.jetty.session.SessionConfig; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.TypeUtil; public class SessionHandler extends AbstractSessionManager implements Handler.Singleton { @@ -569,9 +570,9 @@ public void onSessionDestroyed(Session session) getSessionContext().run(() -> { HttpSessionEvent event = new HttpSessionEvent(session.getApi()); - for (int i = _sessionListeners.size() - 1; i >= 0; i--) + for (HttpSessionListener listener : TypeUtil.reverse(_sessionListeners)) { - _sessionListeners.get(i).sessionDestroyed(event); + listener.sessionDestroyed(event); } }); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java index e01ad6a661b0..3e04fbf1277a 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java @@ -1000,17 +1000,17 @@ protected void requestDestroyed(Request baseRequest, HttpServletRequest request) if (!_servletRequestListeners.isEmpty()) { final ServletRequestEvent sre = new ServletRequestEvent(_apiContext, request); - for (int i = _servletRequestListeners.size(); i-- > 0; ) + for (ServletRequestListener listener : TypeUtil.reverse(_servletRequestListeners)) { - _servletRequestListeners.get(i).requestDestroyed(sre); + listener.requestDestroyed(sre); } } if (!_servletRequestAttributeListeners.isEmpty()) { - for (int i = _servletRequestAttributeListeners.size(); i-- > 0; ) + for (ServletRequestAttributeListener listener : TypeUtil.reverse(_servletRequestAttributeListeners)) { - baseRequest.removeEventListener(_servletRequestAttributeListeners.get(i)); + baseRequest.removeEventListener(listener); } } } @@ -1070,11 +1070,11 @@ protected void exitScope(Request request) { if (!_contextListeners.isEmpty()) { - for (int i = _contextListeners.size(); i-- > 0; ) + for (ContextScopeListener listener : TypeUtil.reverse(_contextListeners)) { try { - _contextListeners.get(i).exitScope(_apiContext, request); + listener.exitScope(_apiContext, request); } catch (Throwable e) { diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java index 9c2848512ea2..d49960cb0e61 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java @@ -54,6 +54,7 @@ import org.eclipse.jetty.session.SessionManager; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.TypeUtil; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -834,9 +835,9 @@ public void onSessionDestroyed(Session session) Runnable r = () -> { HttpSessionEvent event = new HttpSessionEvent(session.getApi()); - for (int i = _sessionListeners.size() - 1; i >= 0; i--) + for (HttpSessionListener listener : TypeUtil.reverse(_sessionListeners)) { - _sessionListeners.get(i).sessionDestroyed(event); + listener.sessionDestroyed(event); } }; _contextHandler.getCoreContextHandler().getContext().run(r);