This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13261 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit 5fd1f81c9512985ff7bf98c75cc9a004faaa3a14 Author: Joerg Hoh <[email protected]> AuthorDate: Sat Jul 11 21:34:20 2026 +0200 SLING-13261 fix race condition on servlet resolution --- .../resolver/internal/SlingServletResolver.java | 4 +- .../internal/resolution/ResolutionCache.java | 61 +++-- .../internal/ResolutionCacheRaceConditionTest.java | 273 +++++++++++++++++++++ 3 files changed, 322 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java index 9b61dcd..d469470 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java @@ -558,6 +558,8 @@ public class SlingServletResolver implements ServletResolver, SlingJakartaReques final ResourceResolver resolver) { // use local variable to avoid race condition with activate final ResolutionCache localCache = this.resolutionCache; + // capture before resolving, so a flush during resolution rejects the stale result + final long cacheGeneration = localCache.getGeneration(); final Servlet scriptServlet = localCache.get(locationUtil); if (scriptServlet != null) { if (LOGGER.isDebugEnabled()) { @@ -593,7 +595,7 @@ public class SlingServletResolver implements ServletResolver, SlingJakartaReques !isOptingServlet || (request != null && ((JakartaOptingServlet) candidate).accepts(request)); if (servletAcceptsRequest) { if (!hasOptingServlet && !isOptingServlet) { - localCache.put(locationUtil, candidate); + localCache.put(cacheGeneration, locationUtil, candidate); } LOGGER.debug("Using servlet provided by candidate resource {}", candidateResource.getPath()); return candidate; diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java b/src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java index c4ce911..55918c7 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/resolution/ResolutionCache.java @@ -30,7 +30,10 @@ import java.util.Hashtable; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import jakarta.servlet.Servlet; import org.apache.sling.api.resource.observation.ExternalResourceChangeListener; @@ -78,6 +81,12 @@ public class ResolutionCache /** The script resolution cache. */ private final AtomicReference<Map<AbstractResourceCollector, Servlet>> cache = new AtomicReference<>(); + /** Incremented on every flush; used to reject entries resolved before a flush. */ + private final AtomicLong generation = new AtomicLong(); + + /** Shared by puts, held exclusively by flushes, so a generation check and insert cannot straddle a flush. */ + private final ReadWriteLock cacheLock = new ReentrantReadWriteLock(); + /** The cache size. */ private volatile int cacheSize; @@ -228,11 +237,17 @@ public class ResolutionCache } public void flushCache() { - // use local variable to avoid racing with deactivate - final Map<AbstractResourceCollector, Servlet> localCache = this.cache.get(); - if (localCache != null) { - localCache.clear(); - this.logCacheSizeWarning = true; + this.cacheLock.writeLock().lock(); + try { + this.generation.incrementAndGet(); + // use local variable to avoid racing with deactivate + final Map<AbstractResourceCollector, Servlet> localCache = this.cache.get(); + if (localCache != null) { + localCache.clear(); + this.logCacheSizeWarning = true; + } + } finally { + this.cacheLock.writeLock().unlock(); } } @@ -278,17 +293,33 @@ public class ResolutionCache return null; } - public void put(final AbstractResourceCollector context, final Servlet candidate) { - final Map<AbstractResourceCollector, Servlet> localCache = this.cache.get(); - if (localCache != null) { - if (localCache.size() < this.cacheSize) { - localCache.put(context, candidate); - } else if (this.logCacheSizeWarning) { - this.logCacheSizeWarning = false; - logger.warn( - "Script cache has reached its limit of {}. You might want to increase the cache size for the servlet resolver.", - this.cacheSize); + /** + * @return the current cache generation, to be passed to {@link #put(long, AbstractResourceCollector, Servlet)} + */ + public long getGeneration() { + return this.generation.get(); + } + + /** + * Caches the candidate unless the cache has been flushed since {@code generation} was obtained, + * i.e. the resolution that produced the candidate may be based on stale data. + */ + public void put(final long generation, final AbstractResourceCollector context, final Servlet candidate) { + this.cacheLock.readLock().lock(); + try { + final Map<AbstractResourceCollector, Servlet> localCache = this.cache.get(); + if (localCache != null && this.generation.get() == generation) { + if (localCache.size() < this.cacheSize) { + localCache.put(context, candidate); + } else if (this.logCacheSizeWarning) { + this.logCacheSizeWarning = false; + logger.warn( + "Script cache has reached its limit of {}. You might want to increase the cache size for the servlet resolver.", + this.cacheSize); + } } + } finally { + this.cacheLock.readLock().unlock(); } } } diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/ResolutionCacheRaceConditionTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/ResolutionCacheRaceConditionTest.java new file mode 100644 index 0000000..94fa56c --- /dev/null +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/ResolutionCacheRaceConditionTest.java @@ -0,0 +1,273 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.servlets.resolver.internal; + +import java.lang.reflect.Field; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +import jakarta.servlet.Servlet; +import org.apache.sling.api.SlingJakartaHttpServletRequest; +import org.apache.sling.api.request.RequestPathInfo; +import org.apache.sling.api.request.RequestProgressTracker; +import org.apache.sling.api.resource.Resource; +import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.wrappers.ResourceResolverWrapper; +import org.apache.sling.servlets.resolver.internal.helper.AbstractResourceCollector; +import org.apache.sling.servlets.resolver.internal.helper.ResourceCollector; +import org.apache.sling.servlets.resolver.internal.resolution.ResolutionCache; +import org.apache.sling.servlets.resolver.internal.resource.MockJakartaServletResource; +import org.apache.sling.testing.resourceresolver.DefaultMockResourceFactory; +import org.apache.sling.testing.resourceresolver.MockResourceResolverFactory; +import org.apache.sling.testing.resourceresolver.MockResourceResolverFactoryOptions; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.osgi.framework.BundleContext; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Reproduces the "stale servlet served from the resolution cache" problem: when two servlets share + * the same {@link ResolutionCache} key (same resource type, method and extension) and the correct one + * registers after the other, {@code SlingServletResolver.getServletInternal()} resolves and caches + * with a non-atomic <em>get → resolve → put</em>. If a {@link ResolutionCache#flushCache()} + * triggered by that registration lands between the resolve and the put, the resolution re-inserts the + * now-stale servlet into the just-flushed cache, and it is served to later requests until an unrelated + * flush clears it. The test forces the flush to occur mid-resolution to trigger this deterministically. + */ +public class ResolutionCacheRaceConditionTest { + + /** Custom factory turning any {@code *.servlet} resource into a servlet-backed resource. */ + private static final class ServletMockResourceFactory extends DefaultMockResourceFactory { + @Override + public Resource newMockResource(String path, Map<String, Object> properties, ResourceResolver resolver) { + if (path.endsWith(".servlet")) { + Servlet servlet = (Servlet) properties.get(MockJakartaServletResource.PROP_SERVLET); + return new MockJakartaServletResource(resolver, servlet, path); + } + return super.newMockResource(path, properties, resolver); + } + } + + private static final String RESOURCE_TYPE = "foo"; + private static final String SCRIPT_PATH = "/foo/foo.servlet"; + + private SlingServletResolver servletResolver; + private ResolutionCache resolutionCache; + private ResourceResolver mockResourceResolver; + + /** Fired once, during candidate collection of a resolution, to simulate a concurrent flush. */ + private Runnable duringResolutionHook; + + @Before + public void setUp() throws Exception { + // ResolverConfig is an annotation type, which Mockito cannot mock on recent JDKs; build a + // real implementation via a dynamic proxy returning the values we care about. + final ResolverConfig config = newResolverConfig(); + + final MockResourceResolverFactoryOptions options = new MockResourceResolverFactoryOptions() + .setSearchPaths(new String[] {"/"}) + .setMockResourceFactory(new ServletMockResourceFactory()); + final MockResourceResolverFactory factory = new MockResourceResolverFactory(options); + + // Wrap the resolver so we can run a hook the first time a resolution touches the tree. + final ResourceResolver realResolver = factory.getResourceResolver(null); + final AtomicBoolean hookFired = new AtomicBoolean(false); + mockResourceResolver = new ResourceResolverWrapper(realResolver) { + private void maybeFire() { + if (duringResolutionHook != null && hookFired.compareAndSet(false, true)) { + duringResolutionHook.run(); + } + } + + @Override + public Resource getResource(String path) { + maybeFire(); + return super.getResource(path); + } + + @Override + public Resource getResource(Resource base, String path) { + maybeFire(); + return super.getResource(base, path); + } + }; + + // Activate a real resolution cache (bundle context is only used for whiteboard registrations). + final BundleContext bundleContext = Mockito.mock(BundleContext.class); + resolutionCache = new ResolutionCache(); + final java.lang.reflect.Method activate = + ResolutionCache.class.getDeclaredMethod("activate", BundleContext.class, ResolverConfig.class); + activate.setAccessible(true); + activate.invoke(resolutionCache, bundleContext, config); + + servletResolver = new SlingServletResolver(); + setField(servletResolver, "resourceResolverFactory", factory); + setField(servletResolver, "resolutionCache", resolutionCache); + setField(servletResolver, "executionPaths", new AtomicReference<String[]>(null)); + setField(servletResolver, "defaultExtensions", new AtomicReference<>(Arrays.asList("html"))); + setField(servletResolver, "useResourceCaching", Boolean.FALSE); + setField(servletResolver, "sharedScriptResolver", new AtomicReference<>(mockResourceResolver)); + } + + /** + * Positive control: without any overlap, registering the correct servlet (modelled by a flush) + * cleanly invalidates the cache. This passes today and proves the flush mechanism itself is fine. + */ + @Test + public void testFlushClearsCacheWhenNoConcurrentResolution() throws Exception { + final Servlet oldServlet = registerServlet(SCRIPT_PATH, "old-servlet"); + + final SlingJakartaHttpServletRequest request = newRequest(RESOURCE_TYPE, "GET", "html"); + assertSame("sanity: first resolution returns the only registered servlet", oldServlet, resolve(request)); + + // entry is now cached + assertNotNull("resolution should have been cached", resolutionCache.get(cacheKey())); + + // second servlet registers -> cache flush (no resolution in flight) + resolutionCache.flushCache(); + + assertNull("a plain flush must remove the stale entry", resolutionCache.get(cacheKey())); + } + + /** + * The bug: a flush that happens <em>while a resolution is in flight</em> is defeated by the + * resolution's trailing {@code cache.put(...)}, leaving the stale servlet cached. + * + * <p>On the current code this assertion FAILS - {@code cacheKey()} still maps to the old servlet, + * which is exactly the servlet that would be (wrongly) served to every following request. + */ + @Test + public void testResolutionOverlappingFlushMustNotRepopulateStaleEntry() throws Exception { + final Servlet oldServlet = registerServlet(SCRIPT_PATH, "old-servlet"); + + // Simulate the correct servlet registering concurrently: during candidate collection of this + // very resolution, its registration fires a cache invalidation (flushCache()). + duringResolutionHook = () -> resolutionCache.flushCache(); + + final SlingJakartaHttpServletRequest request = newRequest(RESOURCE_TYPE, "GET", "html"); + final Servlet resolved = resolve(request); + assertSame("this resolution still legitimately returns the old servlet", oldServlet, resolved); + + // The invalidation happened after this resolution started, so nothing it observed may remain + // cached. Otherwise the stale servlet is served to all later requests. + assertNull( + "A resolution that overlapped a cache flush must not leave a stale entry behind; " + + "the resolutionCache still returns the outdated servlet, which is the " + + "root cause of the intermittent 'wrong servlet executed' symptom.", + resolutionCache.get(cacheKey())); + } + + // ---------- helpers ---------- + + private Servlet resolve(final SlingJakartaHttpServletRequest request) { + return servletResolver.resolve(request); + } + + /** Rebuilds the exact {@link ResourceCollector} key the resolver uses internally for the request. */ + private AbstractResourceCollector cacheKey() { + final Resource resource = mock(Resource.class); + when(resource.getResourceType()).thenReturn(RESOURCE_TYPE); + when(resource.getResourceSuperType()).thenReturn(null); + return ResourceCollector.create(resource, "html", null, Arrays.asList("html"), "GET", new String[0], false); + } + + private Servlet registerServlet(final String path, final String name) throws Exception { + final Servlet servlet = mock(Servlet.class, name); + final Map<String, Object> props = new HashMap<>(); + props.put(MockJakartaServletResource.PROP_SERVLET, servlet); + // remove a previous registration if present + final Resource existing = mockResourceResolver.getResource(path); + if (existing != null) { + mockResourceResolver.delete(existing); + } + Resource parent = mockResourceResolver.getResource("/"); + final String[] segments = path.substring(1).split("/"); + for (int i = 0; i < segments.length - 1; i++) { + Resource child = parent.getChild(segments[i]); + parent = child != null ? child : mockResourceResolver.create(parent, segments[i], null); + } + mockResourceResolver.create(parent, segments[segments.length - 1], props); + mockResourceResolver.commit(); + return servlet; + } + + private SlingJakartaHttpServletRequest newRequest( + final String resourceType, final String method, final String extension) { + final SlingJakartaHttpServletRequest request = mock(SlingJakartaHttpServletRequest.class); + final Resource resource = mock(Resource.class); + final RequestProgressTracker tracker = mock(RequestProgressTracker.class); + final RequestPathInfo pathInfo = mock(RequestPathInfo.class); + + when(request.getResource()).thenReturn(resource); + when(request.getRequestProgressTracker()).thenReturn(tracker); + when(request.getRequestPathInfo()).thenReturn(pathInfo); + when(request.getMethod()).thenReturn(method); + when(pathInfo.getExtension()).thenReturn(extension); + when(pathInfo.getSelectors()).thenReturn(new String[0]); + when(resource.getResourceType()).thenReturn(resourceType); + when(resource.getResourceSuperType()).thenReturn(null); + when(resource.getPath()).thenReturn("/content/test"); + return request; + } + + /** Builds a {@link ResolverConfig} with the values relevant to this test and defaults otherwise. */ + private static ResolverConfig newResolverConfig() { + final Map<String, Object> values = new HashMap<>(); + values.put("servletresolver_servletRoot", "0"); + values.put("servletresolver_cacheSize", 200); + values.put("servletresolver_paths", new String[] {"/"}); + values.put("servletresolver_defaultExtensions", new String[] {"html"}); + values.put("servletresolver_mountProviders", true); + values.put("servletresolver_mountPathProviders", false); + values.put("enable_resource_caching", false); + return (ResolverConfig) java.lang.reflect.Proxy.newProxyInstance( + ResolverConfig.class.getClassLoader(), new Class<?>[] {ResolverConfig.class}, (proxy, method, args) -> { + if ("annotationType".equals(method.getName())) { + return ResolverConfig.class; + } + return values.get(method.getName()); + }); + } + + private static void setField(final Object target, final String name, final Object value) throws Exception { + final Field field = findField(target.getClass(), name); + field.setAccessible(true); + field.set(target, value); + } + + private static Field findField(Class<?> type, final String name) throws NoSuchFieldException { + while (type != null) { + try { + return type.getDeclaredField(name); + } catch (final NoSuchFieldException e) { + type = type.getSuperclass(); + } + } + throw new NoSuchFieldException(name); + } +}
