You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by jo...@apache.org on 2026/07/11 19:34:37 UTC
(sling-org-apache-sling-servlets-resolver) branch SLING-13261 created (now 5fd1f81)
This is an automated email from the ASF dual-hosted git repository.
joerghoh pushed a change to branch SLING-13261
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
at 5fd1f81 SLING-13261 fix race condition on servlet resolution
This branch includes the following new commits:
new 5fd1f81 SLING-13261 fix race condition on servlet resolution
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(sling-org-apache-sling-servlets-resolver) 01/01: SLING-13261 fix race condition on servlet resolution
Posted by jo...@apache.org.
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 <jo...@apache.org>
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);
+ }
+}