From d21edfcddc276ef158d7e44b0adead61dc6fb53d Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 15 Sep 2020 17:14:00 +0200 Subject: [PATCH 1/4] Issue #5022 Filter Cache cleanup Issue #5022 Filter Cache cleanup: + Fixed many compiler warnings + removed old LazyList leftovers + Don't create holder string for source unless required + Only have a single type of chain, so it can be wrapped regardless of cache + Reverse mappings lists to make filter chain creation easier + build chain directly rather than build a list then a chain Signed-off-by: Greg Wilkins --- .../org/eclipse/jetty/server/Request.java | 4 +- .../eclipse/jetty/servlet/ServletHandler.java | 384 +++++++----------- 2 files changed, 148 insertions(+), 240 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 9417e5c37a33..07ad7e4be04d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -195,7 +195,7 @@ public static Request getBaseRequest(ServletRequest request) private String _servletPath; private String _pathInfo; private boolean _secure; - private String _asyncNotSupportedSource = null; + private Object _asyncNotSupportedSource = null; private boolean _newContext; private boolean _cookiesExtracted = false; private boolean _handled = false; @@ -1940,7 +1940,7 @@ public void removeEventListener(final EventListener listener) _requestAttributeListeners.remove(listener); } - public void setAsyncSupported(boolean supported, String source) + public void setAsyncSupported(boolean supported, Object source) { _asyncNotSupportedSource = supported ? null : (source == null ? "unknown" : source); } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 00e34d91fed1..9c642094cee4 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.ListIterator; import java.util.Map; +import java.util.Objects; import java.util.Queue; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -63,7 +64,6 @@ import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ScopedHandler; import org.eclipse.jetty.util.ArrayUtil; -import org.eclipse.jetty.util.LazyList; import org.eclipse.jetty.util.MultiException; import org.eclipse.jetty.util.MultiMap; import org.eclipse.jetty.util.URIUtil; @@ -112,6 +112,7 @@ public class ServletHandler extends ScopedHandler private final Map _filterNameMap = new HashMap<>(); private List _filterPathMappings; private MultiMap _filterNameMappings; + private List _wildFilterNameMappings; private final Map _servletNameMap = new HashMap<>(); // private PathMap _servletPathMap; @@ -136,7 +137,7 @@ public ServletHandler() @Override public boolean isDumpable(Object o) { - return !(o instanceof Holder || o instanceof BaseHolder || o instanceof FilterMapping || o instanceof ServletMapping); + return !(o instanceof BaseHolder || o instanceof FilterMapping || o instanceof ServletMapping); } @Override @@ -274,14 +275,14 @@ protected synchronized void doStop() } //Retain only filters and mappings that were added using jetty api (ie Source.EMBEDDED) - FilterHolder[] fhs = (FilterHolder[])LazyList.toArray(filterHolders, FilterHolder.class); + FilterHolder[] fhs = filterHolders.toArray(new FilterHolder[0]); updateBeans(_filters, fhs); _filters = fhs; - FilterMapping[] fms = (FilterMapping[])LazyList.toArray(filterMappings, FilterMapping.class); + FilterMapping[] fms = filterMappings.toArray(new FilterMapping[0]); updateBeans(_filterMappings, fms); _filterMappings = fms; - _matchAfterIndex = (_filterMappings == null || _filterMappings.length == 0 ? -1 : _filterMappings.length - 1); + _matchAfterIndex = (_filterMappings.length == 0 ? -1 : _filterMappings.length - 1); _matchBeforeIndex = -1; // Stop servlets @@ -319,10 +320,10 @@ protected synchronized void doStop() } //Retain only Servlets and mappings added via jetty apis (ie Source.EMBEDDED) - ServletHolder[] shs = (ServletHolder[])LazyList.toArray(servletHolders, ServletHolder.class); + ServletHolder[] shs = servletHolders.toArray(new ServletHolder[0]); updateBeans(_servlets, shs); _servlets = shs; - ServletMapping[] sms = (ServletMapping[])LazyList.toArray(servletMappings, ServletMapping.class); + ServletMapping[] sms = servletMappings.toArray(new ServletMapping[0]); updateBeans(_servletMappings, sms); _servletMappings = sms; @@ -347,7 +348,7 @@ protected synchronized void doStop() listenerHolders.add(_listeners[i]); } } - ListenerHolder[] listeners = (ListenerHolder[])LazyList.toArray(listenerHolders, ListenerHolder.class); + ListenerHolder[] listeners = listenerHolders.toArray(new ListenerHolder[0]); updateBeans(_listeners, listeners); _listeners = listeners; @@ -591,69 +592,69 @@ public MappedResource getMappedServlet(String target) return _servletPathMap.getMatch(target); } - if (_servletNameMap == null) - return null; ServletHolder holder = _servletNameMap.get(target); if (holder == null) return null; return new MappedResource<>(null, holder); } + private FilterChain linkFilterChain(ServletHolder servletHolder, FilterHolder filterHolder, FilterChain chain) + { + if (chain == null) + chain = new ServletFilterChain(servletHolder); + FilterChain next = newFilterChain(filterHolder.getFilter(), chain); + return filterHolder.isAsyncSupported() ? next : new NotAsyncFilterChain(filterHolder, chain); + } + + /** + * Create a FilterChain that calls the passed filter with the passed chain + * @param filter The filter to invoke + * @param chain The chain to pass to the filter + * @return A FilterChain that invokes the filter with the chain + */ + protected FilterChain newFilterChain(Filter filter, FilterChain chain) + { + return new Chain(filter, chain); + } + protected FilterChain getFilterChain(Request baseRequest, String pathInContext, ServletHolder servletHolder) { String key = pathInContext == null ? servletHolder.getName() : pathInContext; int dispatch = FilterMapping.dispatch(baseRequest.getDispatcherType()); - if (_filterChainsCached && _chainCache != null) + if (_filterChainsCached) { FilterChain chain = _chainCache[dispatch].get(key); if (chain != null) return chain; } - // Build list of filters (list of FilterHolder objects) - List filters = new ArrayList<>(); - - // Path filters - if (pathInContext != null && _filterPathMappings != null) - { - for (FilterMapping filterPathMapping : _filterPathMappings) - { - if (filterPathMapping.appliesTo(pathInContext, dispatch)) - filters.add(filterPathMapping.getFilterHolder()); - } - } + FilterChain chain = null; - // Servlet name filters if (servletHolder != null && _filterNameMappings != null && !_filterNameMappings.isEmpty()) { - Object o = _filterNameMappings.get(servletHolder.getName()); + if (_wildFilterNameMappings != null) + for (FilterMapping mapping : _wildFilterNameMappings) + chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); - for (int i = 0; i < LazyList.size(o); i++) + for (FilterMapping mapping : _filterNameMappings.get(servletHolder.getName())) { - FilterMapping mapping = LazyList.get(o, i); if (mapping.appliesTo(dispatch)) - filters.add(mapping.getFilterHolder()); + chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); } + } - o = _filterNameMappings.get("*"); - for (int i = 0; i < LazyList.size(o); i++) + if (pathInContext != null && _filterPathMappings != null) + { + for (FilterMapping mapping : _filterPathMappings) { - FilterMapping mapping = LazyList.get(o, i); - if (mapping.appliesTo(dispatch)) - filters.add(mapping.getFilterHolder()); + if (mapping.appliesTo(pathInContext, dispatch)) + chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); } } - if (filters.isEmpty()) - return null; - - FilterChain chain = null; if (_filterChainsCached) { - if (!filters.isEmpty()) - chain = newCachedChain(filters, servletHolder); - final Map cache = _chainCache[dispatch]; final Queue lru = _chainLRU[dispatch]; @@ -672,12 +673,11 @@ protected FilterChain getFilterChain(Request baseRequest, String pathInContext, cache.remove(k); } + if (chain == null) + chain = new ServletFilterChain(servletHolder); cache.put(key, chain); lru.add(key); } - else if (!filters.isEmpty()) - chain = new Chain(baseRequest, filters, servletHolder); - return chain; } @@ -841,18 +841,6 @@ public ListenerHolder newListenerHolder(Source source) return new ListenerHolder(source); } - /** - * Create a new CachedChain - * - * @param filters the filter chain to be cached as a collection of {@link FilterHolder} - * @param servletHolder the servletHolder - * @return a new {@link CachedChain} instance - */ - public CachedChain newCachedChain(List filters, ServletHolder servletHolder) - { - return new CachedChain(filters, servletHolder); - } - /** * Add a new servlet holder * @@ -903,6 +891,7 @@ public ServletHolder addServletWithMapping(Class servlet, Str */ public void addServletWithMapping(ServletHolder servlet, String pathSpec) { + Objects.requireNonNull(servlet); ServletHolder[] holders = getServlets(); if (holders != null) holders = holders.clone(); @@ -911,7 +900,7 @@ public void addServletWithMapping(ServletHolder servlet, String pathSpec) { synchronized (this) { - if (servlet != null && !containsServletHolder(servlet)) + if (!containsServletHolder(servlet)) setServlets(ArrayUtil.addToArray(holders, servlet, ServletHolder.class)); } @@ -1016,6 +1005,7 @@ public FilterHolder addFilterWithMapping(String className, String pathSpec, Enum */ public void addFilterWithMapping(FilterHolder holder, String pathSpec, EnumSet dispatches) { + Objects.requireNonNull(holder); FilterHolder[] holders = getFilters(); if (holders != null) holders = holders.clone(); @@ -1024,7 +1014,7 @@ public void addFilterWithMapping(FilterHolder holder, String pathSpec, EnumSet> entry : _filterNameMappings.entrySet()) + Collections.reverse(entry.getValue()); + Collections.reverse(_filterPathMappings); + _wildFilterNameMappings = _filterNameMappings.get("*"); + if (_wildFilterNameMappings != null) + Collections.reverse(_wildFilterNameMappings); } // Map servlet paths to holders - if (_servletMappings == null || _servletNameMap == null) + if (_servletMappings == null) { _servletPathMap = null; } else { PathMappings pm = new PathMappings<>(); - Map servletPathMappings = new HashMap<>(); //create a map of paths to set of ServletMappings that define that mapping HashMap> sms = new HashMap<>(); @@ -1381,12 +1380,7 @@ protected synchronized void updateMappings() { for (String pathSpec : pathSpecs) { - List mappings = sms.get(pathSpec); - if (mappings == null) - { - mappings = new ArrayList<>(); - sms.put(pathSpec, mappings); - } + List mappings = sms.computeIfAbsent(pathSpec, k -> new ArrayList<>()); mappings.add(servletMapping); } } @@ -1448,7 +1442,6 @@ else if (isAllowDuplicateMappings()) finalMapping.getServletName(), _servletNameMap.get(finalMapping.getServletName()).getSource()); - servletPathMappings.put(pathSpec, finalMapping); pm.put(new ServletPathSpec(pathSpec), _servletNameMap.get(finalMapping.getServletName())); } @@ -1456,13 +1449,10 @@ else if (isAllowDuplicateMappings()) } // flush filter chain cache - if (_chainCache != null) + for (int i = _chainCache.length; i-- > 0; ) { - for (int i = _chainCache.length; i-- > 0; ) - { - if (_chainCache[i] != null) - _chainCache[i].clear(); - } + if (_chainCache[i] != null) + _chainCache[i].clear(); } if (LOG.isDebugEnabled()) @@ -1497,28 +1487,26 @@ protected synchronized boolean containsFilterHolder(FilterHolder holder) { if (_filters == null) return false; - boolean found = false; for (FilterHolder f : _filters) { if (f == holder) - found = true; + return true; } - return found; + return false; } protected synchronized boolean containsServletHolder(ServletHolder holder) { if (_servlets == null) return false; - boolean found = false; for (ServletHolder s : _servlets) { @SuppressWarnings("ReferenceEquality") boolean foundServletHolder = (s == holder); if (foundServletHolder) - found = true; + return true; } - return found; + return false; } /** @@ -1584,161 +1572,6 @@ public synchronized void setServlets(ServletHolder[] holders) invalidateChainsCache(); } - protected class CachedChain implements FilterChain - { - FilterHolder _filterHolder; - CachedChain _next; - ServletHolder _servletHolder; - - /** - * @param filters list of {@link FilterHolder} objects - * @param servletHolder the current {@link ServletHolder} - */ - protected CachedChain(List filters, ServletHolder servletHolder) - { - if (!filters.isEmpty()) - { - _filterHolder = filters.get(0); - filters.remove(0); - _next = new CachedChain(filters, servletHolder); - } - else - _servletHolder = servletHolder; - } - - @Override - public void doFilter(ServletRequest request, ServletResponse response) - throws IOException, ServletException - { - final Request baseRequest = Request.getBaseRequest(request); - - // pass to next filter - if (_filterHolder != null) - { - if (LOG.isDebugEnabled()) - LOG.debug("call filter {}", _filterHolder); - Filter filter = _filterHolder.getFilter(); - - //if the request already does not support async, then the setting for the filter - //is irrelevant. However if the request supports async but this filter does not - //temporarily turn it off for the execution of the filter - if (baseRequest.isAsyncSupported() && !_filterHolder.isAsyncSupported()) - { - try - { - baseRequest.setAsyncSupported(false, _filterHolder.toString()); - filter.doFilter(request, response, _next); - } - finally - { - baseRequest.setAsyncSupported(true, null); - } - } - else - filter.doFilter(request, response, _next); - - return; - } - - // Call servlet - HttpServletRequest srequest = (HttpServletRequest)request; - if (_servletHolder == null) - notFound(baseRequest, srequest, (HttpServletResponse)response); - else - { - if (LOG.isDebugEnabled()) - LOG.debug("call servlet " + _servletHolder); - _servletHolder.handle(baseRequest, request, response); - } - } - - @Override - public String toString() - { - if (_filterHolder != null) - return _filterHolder + "->" + _next.toString(); - if (_servletHolder != null) - return _servletHolder.toString(); - return "null"; - } - } - - private class Chain implements FilterChain - { - final Request _baseRequest; - final List _chain; - final ServletHolder _servletHolder; - int _filter = 0; - - private Chain(Request baseRequest, List filters, ServletHolder servletHolder) - { - _baseRequest = baseRequest; - _chain = filters; - _servletHolder = servletHolder; - } - - @Override - public void doFilter(ServletRequest request, ServletResponse response) - throws IOException, ServletException - { - if (LOG.isDebugEnabled()) - LOG.debug("doFilter " + _filter); - - // pass to next filter - if (_filter < _chain.size()) - { - FilterHolder holder = _chain.get(_filter++); - if (LOG.isDebugEnabled()) - LOG.debug("call filter " + holder); - Filter filter = holder.getFilter(); - - //if the request already does not support async, then the setting for the filter - //is irrelevant. However if the request supports async but this filter does not - //temporarily turn it off for the execution of the filter - if (!holder.isAsyncSupported() && _baseRequest.isAsyncSupported()) - { - try - { - _baseRequest.setAsyncSupported(false, holder.toString()); - filter.doFilter(request, response, this); - } - finally - { - _baseRequest.setAsyncSupported(true, null); - } - } - else - filter.doFilter(request, response, this); - - return; - } - - // Call servlet - HttpServletRequest srequest = (HttpServletRequest)request; - if (_servletHolder == null) - notFound(Request.getBaseRequest(request), srequest, (HttpServletResponse)response); - else - { - if (LOG.isDebugEnabled()) - LOG.debug("call servlet {}", _servletHolder); - _servletHolder.handle(_baseRequest, request, response); - } - } - - @Override - public String toString() - { - StringBuilder b = new StringBuilder(); - for (FilterHolder f : _chain) - { - b.append(f.toString()); - b.append("->"); - } - b.append(_servletHolder); - return b.toString(); - } - } - /** * @return The maximum entries in a filter chain cache. */ @@ -1787,4 +1620,79 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) resp.sendError(HttpServletResponse.SC_NOT_FOUND); } } + + static class Chain implements FilterChain + { + private final Filter _filter; + private final FilterChain _chain; + + Chain(Filter filter, FilterChain chain) + { + _filter = filter; + _chain = chain; + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException + { + _filter.doFilter(request, response, _chain); + } + + // TODO toString + } + + static class ServletFilterChain implements FilterChain + { + private final ServletHolder _holder; + + ServletFilterChain(ServletHolder holder) + { + _holder = holder; + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException + { + Request baseRequest = Request.getBaseRequest(request); + Objects.requireNonNull(baseRequest); + _holder.handle(baseRequest, request, response); + } + + // TODO toString + } + + static class NotAsyncFilterChain implements FilterChain + { + private final FilterHolder _holder; + private final FilterChain _chain; + + NotAsyncFilterChain(FilterHolder holder, FilterChain chain) + { + _holder = holder; + _chain = chain; + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException + { + if (!request.isAsyncSupported()) + _holder.getFilter().doFilter(request, response, _chain); + else + { + Request baseRequest = Request.getBaseRequest(request); + Objects.requireNonNull(baseRequest); + try + { + baseRequest.setAsyncSupported(false, _holder); + _holder.getFilter().doFilter(request, response, _chain); + } + finally + { + baseRequest.setAsyncSupported(true, null); + } + } + } + + // TODO toString + } } From 006659c336b1f1faf793883509d4c1921d49fcab Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 15 Sep 2020 20:51:47 +0200 Subject: [PATCH 2/4] added comment to explain ordering Signed-off-by: gregw --- .../main/java/org/eclipse/jetty/servlet/ServletHandler.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 9c642094cee4..2f0ff926cf52 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -629,6 +629,9 @@ protected FilterChain getFilterChain(Request baseRequest, String pathInContext, return chain; } + // Build the filter chain from the inside out. + // ie first wrap the servlet with the last filter to be applied. + // The mappings lists have been reversed to make this simple and fast. FilterChain chain = null; if (servletHolder != null && _filterNameMappings != null && !_filterNameMappings.isEmpty()) From 52b9925a0f68a287d468a1f82e8c7ba9688ae317 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 15 Sep 2020 22:03:17 +0200 Subject: [PATCH 3/4] More cleanups --- .../eclipse/jetty/servlet/FilterHolder.java | 25 +++ .../eclipse/jetty/servlet/ServletHandler.java | 145 +++++------------- 2 files changed, 66 insertions(+), 104 deletions(-) diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java index 38682af0da95..1a0f5bcee921 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java @@ -24,13 +24,18 @@ import java.util.Collection; import java.util.EnumSet; import java.util.List; +import java.util.Objects; import javax.servlet.DispatcherType; import javax.servlet.Filter; +import javax.servlet.FilterChain; import javax.servlet.FilterConfig; import javax.servlet.FilterRegistration; import javax.servlet.ServletContext; import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import org.eclipse.jetty.server.Request; import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.component.Dumpable; import org.eclipse.jetty.util.component.DumpableCollection; @@ -175,6 +180,26 @@ public Filter getFilter() return _filter; } + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException + { + if (isAsyncSupported() || !request.isAsyncSupported()) + getFilter().doFilter(request, response, chain); + else + { + Request baseRequest = Request.getBaseRequest(request); + Objects.requireNonNull(baseRequest); + try + { + baseRequest.setAsyncSupported(false, this); + getFilter().doFilter(request, response, chain); + } + finally + { + baseRequest.setAsyncSupported(true, null); + } + } + } + @Override public void dump(Appendable out, String indent) throws IOException { diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index 2f0ff926cf52..ad064656c5fb 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -29,10 +29,8 @@ import java.util.ListIterator; import java.util.Map; import java.util.Objects; -import java.util.Queue; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.ConcurrentMap; import java.util.function.Consumer; import java.util.stream.Stream; @@ -101,7 +99,7 @@ public class ServletHandler extends ScopedHandler private int _matchBeforeIndex = -1; //index of last programmatic FilterMapping with isMatchAfter=false private int _matchAfterIndex = -1; //index of 1st programmatic FilterMapping with isMatchAfter=true private boolean _filterChainsCached = true; - private int _maxFilterChainsCacheSize = 512; + private int _maxFilterChainsCacheSize = 1024; private boolean _startWithUnavailable = false; private boolean _ensureDefaultServlet = true; private IdentityService _identityService; @@ -115,7 +113,6 @@ public class ServletHandler extends ScopedHandler private List _wildFilterNameMappings; private final Map _servletNameMap = new HashMap<>(); - // private PathMap _servletPathMap; private PathMappings _servletPathMap; private ListenerHolder[] _listeners = new ListenerHolder[0]; @@ -124,9 +121,6 @@ public class ServletHandler extends ScopedHandler @SuppressWarnings("unchecked") protected final ConcurrentMap[] _chainCache = new ConcurrentMap[FilterMapping.ALL]; - @SuppressWarnings("unchecked") - protected final Queue[] _chainLRU = new Queue[FilterMapping.ALL]; - /** * Constructor. */ @@ -185,12 +179,6 @@ protected synchronized void doStart() _chainCache[FilterMapping.INCLUDE] = new ConcurrentHashMap<>(); _chainCache[FilterMapping.ERROR] = new ConcurrentHashMap<>(); _chainCache[FilterMapping.ASYNC] = new ConcurrentHashMap<>(); - - _chainLRU[FilterMapping.REQUEST] = new ConcurrentLinkedQueue<>(); - _chainLRU[FilterMapping.FORWARD] = new ConcurrentLinkedQueue<>(); - _chainLRU[FilterMapping.INCLUDE] = new ConcurrentLinkedQueue<>(); - _chainLRU[FilterMapping.ERROR] = new ConcurrentLinkedQueue<>(); - _chainLRU[FilterMapping.ASYNC] = new ConcurrentLinkedQueue<>(); } if (_contextHandler == null) @@ -598,25 +586,6 @@ public MappedResource getMappedServlet(String target) return new MappedResource<>(null, holder); } - private FilterChain linkFilterChain(ServletHolder servletHolder, FilterHolder filterHolder, FilterChain chain) - { - if (chain == null) - chain = new ServletFilterChain(servletHolder); - FilterChain next = newFilterChain(filterHolder.getFilter(), chain); - return filterHolder.isAsyncSupported() ? next : new NotAsyncFilterChain(filterHolder, chain); - } - - /** - * Create a FilterChain that calls the passed filter with the passed chain - * @param filter The filter to invoke - * @param chain The chain to pass to the filter - * @return A FilterChain that invokes the filter with the chain - */ - protected FilterChain newFilterChain(Filter filter, FilterChain chain) - { - return new Chain(filter, chain); - } - protected FilterChain getFilterChain(Request baseRequest, String pathInContext, ServletHolder servletHolder) { String key = pathInContext == null ? servletHolder.getName() : pathInContext; @@ -638,12 +607,12 @@ protected FilterChain getFilterChain(Request baseRequest, String pathInContext, { if (_wildFilterNameMappings != null) for (FilterMapping mapping : _wildFilterNameMappings) - chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); + chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); for (FilterMapping mapping : _filterNameMappings.get(servletHolder.getName())) { if (mapping.appliesTo(dispatch)) - chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); + chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); } } @@ -652,48 +621,43 @@ protected FilterChain getFilterChain(Request baseRequest, String pathInContext, for (FilterMapping mapping : _filterPathMappings) { if (mapping.appliesTo(pathInContext, dispatch)) - chain = linkFilterChain(servletHolder, mapping.getFilterHolder(), chain); + chain = newFilterChain(mapping.getFilterHolder(), chain == null ? new ChainEnd(servletHolder) : chain); } } if (_filterChainsCached) { final Map cache = _chainCache[dispatch]; - final Queue lru = _chainLRU[dispatch]; - // Do we have too many cached chains? - while (_maxFilterChainsCacheSize > 0 && cache.size() >= _maxFilterChainsCacheSize) + if (_maxFilterChainsCacheSize > 0 && cache.size() >= _maxFilterChainsCacheSize) { - // The LRU list is not atomic with the cache map, so be prepared to invalidate if - // a key is not found to delete. - // Delete by LRU (where U==created) - String k = lru.poll(); - if (k == null) - { - cache.clear(); - break; - } - cache.remove(k); + // flush the cache + LOG.debug("{} flushed filter chain cache for {}", this, baseRequest.getDispatcherType()); + cache.clear(); } - - if (chain == null) - chain = new ServletFilterChain(servletHolder); + chain = chain == null ? new ChainEnd(servletHolder) : chain; + // flush the cache + LOG.debug("{} cached filter chain for {}: {}", this, baseRequest.getDispatcherType(), chain); cache.put(key, chain); - lru.add(key); } return chain; } + /** + * Create a FilterChain that calls the passed filter with the passed chain + * @param filterHolder The filter to invoke + * @param chain The chain to pass to the filter + * @return A FilterChain that invokes the filter with the chain + */ + protected FilterChain newFilterChain(FilterHolder filterHolder, FilterChain chain) + { + return new Chain(filterHolder, chain); + } + protected void invalidateChainsCache() { - if (_chainLRU[FilterMapping.REQUEST] != null) + if (_chainCache[FilterMapping.REQUEST] != null) { - _chainLRU[FilterMapping.REQUEST].clear(); - _chainLRU[FilterMapping.FORWARD].clear(); - _chainLRU[FilterMapping.INCLUDE].clear(); - _chainLRU[FilterMapping.ERROR].clear(); - _chainLRU[FilterMapping.ASYNC].clear(); - _chainCache[FilterMapping.REQUEST].clear(); _chainCache[FilterMapping.FORWARD].clear(); _chainCache[FilterMapping.INCLUDE].clear(); @@ -1626,31 +1590,35 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) static class Chain implements FilterChain { - private final Filter _filter; - private final FilterChain _chain; + private final FilterHolder _filterHolder; + private final FilterChain _filterChain; - Chain(Filter filter, FilterChain chain) + Chain(FilterHolder filter, FilterChain chain) { - _filter = filter; - _chain = chain; + _filterHolder = filter; + _filterChain = chain; } @Override public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException { - _filter.doFilter(request, response, _chain); + _filterHolder.doFilter(request, response, _filterChain); } - // TODO toString + @Override + public String toString() + { + return String.format("Chain@%x(%s)->%s", hashCode(), _filterHolder, _filterChain); + } } - static class ServletFilterChain implements FilterChain + static class ChainEnd implements FilterChain { - private final ServletHolder _holder; + private final ServletHolder _servletHolder; - ServletFilterChain(ServletHolder holder) + ChainEnd(ServletHolder holder) { - _holder = holder; + _servletHolder = holder; } @Override @@ -1658,44 +1626,13 @@ public void doFilter(ServletRequest request, ServletResponse response) throws IO { Request baseRequest = Request.getBaseRequest(request); Objects.requireNonNull(baseRequest); - _holder.handle(baseRequest, request, response); - } - - // TODO toString - } - - static class NotAsyncFilterChain implements FilterChain - { - private final FilterHolder _holder; - private final FilterChain _chain; - - NotAsyncFilterChain(FilterHolder holder, FilterChain chain) - { - _holder = holder; - _chain = chain; + _servletHolder.handle(baseRequest, request, response); } @Override - public void doFilter(ServletRequest request, ServletResponse response) throws IOException, ServletException + public String toString() { - if (!request.isAsyncSupported()) - _holder.getFilter().doFilter(request, response, _chain); - else - { - Request baseRequest = Request.getBaseRequest(request); - Objects.requireNonNull(baseRequest); - try - { - baseRequest.setAsyncSupported(false, _holder); - _holder.getFilter().doFilter(request, response, _chain); - } - finally - { - baseRequest.setAsyncSupported(true, null); - } - } + return String.format("ChainEnd@%x(%s)->%s", hashCode(), _servletHolder); } - - // TODO toString } } From 25a1f9afdced1b6479e46ddc4ac927b1e130cab2 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 16 Sep 2020 16:06:52 +0200 Subject: [PATCH 4/4] fixed toString format turn off debug in OSGI test --- .../org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java | 4 ++-- .../main/java/org/eclipse/jetty/servlet/ServletHandler.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-osgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java b/jetty-osgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java index 1f971fd0e33c..eefd5b85be27 100644 --- a/jetty-osgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java +++ b/jetty-osgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java @@ -82,7 +82,7 @@ public Option[] config() options.add(mavenBundle().groupId("org.eclipse.jetty.http2").artifactId("http2-http-client-transport").versionAsInProject().start()); options.add(systemProperty("org.ops4j.pax.logging.DefaultServiceLog.level").value(LOG_LEVEL)); - options.add(systemProperty("org.eclipse.jetty.LEVEL").value("DEBUG")); + options.add(systemProperty("org.eclipse.jetty.LEVEL").value("INFO")); options.add(CoreOptions.cleanCaches(true)); return options.toArray(new Option[0]); } @@ -154,7 +154,7 @@ public void testHTTP2() throws Exception httpClient.start(); ContentResponse response = httpClient.GET("https://localhost:" + port + "/jsp/jstl.jsp"); - assertEquals(response.toString(), response.getStatus(), HttpStatus.OK_200); + assertEquals(HttpStatus.OK_200,response.getStatus()); String body = response.getContentAsString(); assertTrue("Body contains \"JSTL Example\": " + body, body.contains("JSTL Example")); } diff --git a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java index ad064656c5fb..1364309fae4b 100644 --- a/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java +++ b/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java @@ -1632,7 +1632,7 @@ public void doFilter(ServletRequest request, ServletResponse response) throws IO @Override public String toString() { - return String.format("ChainEnd@%x(%s)->%s", hashCode(), _servletHolder); + return String.format("ChainEnd@%x(%s)", hashCode(), _servletHolder); } } }