This is an automated email from the ASF dual-hosted git repository. jsedding pushed a commit to branch resolver-2.x-backports in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit d3934634416afe382662ceccfce831602bfc4394 Author: Jörg Hoh <[email protected]> AuthorDate: Tue Jul 21 09:42:52 2026 +0200 SLING-13262 avoid inconsistent state with concurrent registrations (#70) (cherry picked from commit 49b5aeca2e14bde14b08dd5f8514e9ca96ea6dae) --- .../internal/bundle/BundledScriptTracker.java | 15 +- .../BundledScriptTrackerRaceConditionTest.java | 209 +++++++++++++++++++++ 2 files changed, 221 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java b/src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java index 05380df..3596e23 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTracker.java @@ -112,7 +112,7 @@ public class BundledScriptTracker implements BundleTrackerCustomizer<List<Servic private BundledRenderUnitFinder bundledRenderUnitFinder; @Reference - private ServletMounter mounter; + ServletMounter mounter; // package-private for tests; injected by the OSGi runtime private final AtomicReference<BundleContext> bundleContext = new AtomicReference<>(); private final AtomicReference<BundleTracker<List<ServiceRegistration<Servlet>>>> tracker = new AtomicReference<>(); @@ -490,7 +490,9 @@ public class BundledScriptTracker implements BundleTrackerCustomizer<List<Servic } } - private void refreshDispatcher(List<ServiceRegistration<Servlet>> regs) { + // addingBundle/removedBundle may be dispatched on different threads, so force a synchronized run. + // package-private rather than private so it can be exercised directly from tests. + synchronized void refreshDispatcher(List<ServiceRegistration<Servlet>> regs) { BundleContext bc = bundleContext.get(); Map<Bundle, List<ServiceRegistration<Servlet>>> tracked; BundleTracker<List<ServiceRegistration<Servlet>>> bt = tracker.get(); @@ -499,7 +501,14 @@ public class BundledScriptTracker implements BundleTrackerCustomizer<List<Servic } else { tracked = Collections.emptyMap(); } - Map<Set<String>, ServiceRegistration<Servlet>> oldDispatchers = dispatchers.get(); + // Work on a copy of the published map: the reuse bookkeeping below removes entries, so mutating the + // published map in place would let other threads observe it half-drained. It is swapped in atomically + // at the end instead. A null map means the tracker has been deactivated - nothing to (re)register. + final Map<Set<String>, ServiceRegistration<Servlet>> published = dispatchers.get(); + if (published == null) { + return; + } + Map<Set<String>, ServiceRegistration<Servlet>> oldDispatchers = new HashMap<>(published); Map<Set<String>, ServiceRegistration<Servlet>> newDispatchers = new HashMap<>(); final Converter c = Converters.standardConverter(); Stream.concat(tracked.values().stream(), Stream.of(regs)) diff --git a/src/test/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTrackerRaceConditionTest.java b/src/test/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTrackerRaceConditionTest.java new file mode 100644 index 0000000..48588b3 --- /dev/null +++ b/src/test/java/org/apache/sling/servlets/resolver/internal/bundle/BundledScriptTrackerRaceConditionTest.java @@ -0,0 +1,209 @@ +/* + * 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.bundle; + +import javax.servlet.Servlet; + +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.BiFunction; + +import org.apache.sling.api.resource.ResourceResolverFactory; +import org.apache.sling.api.servlets.ServletResolverConstants; +import org.apache.sling.servlets.resolver.internal.ResolverConfig; +import org.apache.sling.servlets.resolver.internal.resource.ServletMounter; +import org.junit.Before; +import org.junit.Test; +import org.osgi.framework.Bundle; +import org.osgi.framework.BundleContext; +import org.osgi.framework.ServiceReference; +import org.osgi.framework.ServiceRegistration; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +/** + * Demonstrates that {@code BundledScriptTracker.refreshDispatcher()} is not thread-safe: it is reachable + * concurrently from {@code addingBundle}/{@code removedBundle} and does a non-atomic read/mutate-in-place/swap + * of the shared {@code dispatchers} map, so concurrent invocations drain the same published map against each + * other and register duplicate (or lose) {@code DispatcherServlet}s - in production, bundled scripts then + * intermittently stop resolving or serve a stale script. The test asserts idempotency (refreshing repeatedly + * with the same registrations must register each resource type's dispatcher exactly once); OSGi collaborators + * are stubbed with {@link Proxy} because {@link ServiceReference}/{@link Bundle} extend {@link Comparable}, + * which the mocking engine cannot instrument on recent JDKs. + */ +public class BundledScriptTrackerRaceConditionTest { + + private static final int RESOURCE_TYPE_COUNT = 4; + private static final int THREADS = 4; + private static final int ROUNDS = 200; + + private BundledScriptTracker tracker; + + /** Counts DispatcherServlet registrations / unregistrations, observed through the stubbed OSGi context. */ + private final AtomicInteger registrations = new AtomicInteger(); + + private final AtomicInteger unregistrations = new AtomicInteger(); + + /** The bundle whose context every DispatcherServlet registration is funnelled through. */ + private Bundle registeringBundle; + + @Before + public void setUp() { + final BundleContext context = proxy(BundleContext.class, (method, args) -> { + if ("registerService".equals(method.getName()) && args != null && args.length == 3) { + registrations.incrementAndGet(); + return dispatcherRegistration(); + } + return null; // getBundles()/addBundleListener()/... during activate() + }); + registeringBundle = + proxy(Bundle.class, (method, args) -> "getBundleContext".equals(method.getName()) ? context : null); + + tracker = new BundledScriptTracker(); + tracker.mounter = servletMounter(context); + // activate() publishes an empty dispatcher map and opens the (empty) bundle tracker. + tracker.activate(context); + } + + /** + * Concurrently refresh the dispatcher with the same set of registrations many times. A correct, + * thread-safe implementation registers each resource type's DispatcherServlet exactly once and reuses it + * on every later refresh; the racy implementation registers duplicates (or throws while mutating the + * shared map from multiple threads). + */ + @Test + public void concurrentRefreshMustNotDuplicateOrDropDispatchers() throws Exception { + final List<ServiceRegistration<Servlet>> inputs = new ArrayList<>(); + for (int i = 0; i < RESOURCE_TYPE_COUNT; i++) { + inputs.add(inputRegistration("test/rt" + i + "/1.0.0")); + } + + final List<Throwable> failures = new CopyOnWriteArrayList<>(); + + for (int round = 0; round < ROUNDS && failures.isEmpty(); round++) { + final CyclicBarrier startLine = new CyclicBarrier(THREADS); + final CountDownLatch done = new CountDownLatch(THREADS); + for (int t = 0; t < THREADS; t++) { + new Thread(() -> { + try { + startLine.await(5, TimeUnit.SECONDS); // release all threads together + tracker.refreshDispatcher(new ArrayList<>(inputs)); + } catch (final Throwable th) { + failures.add(th); + } finally { + done.countDown(); + } + }) + .start(); + } + if (!done.await(15, TimeUnit.SECONDS)) { + fail("refreshDispatcher deadlocked/looped on the shared HashMap - a symptom of the race"); + } + } + + if (!failures.isEmpty()) { + fail("refreshDispatcher threw under concurrency (shared-state corruption): " + failures.get(0)); + } + + // Each resource type's dispatcher must be registered exactly once and then reused; extra registrations + // mean concurrent refreshes drained the shared map and re-registered duplicates. + assertEquals( + "DispatcherServlet must be registered exactly once per resource type", + RESOURCE_TYPE_COUNT, + registrations.get()); + assertEquals( + "no live DispatcherServlet must be unregistered when the desired set is unchanged", + 0, + unregistrations.get()); + } + + // ---------- stubs ---------- + + /** A real ServletMounter with a null 'provider' so mountProviders() is true and register() uses the context. */ + private static ServletMounter servletMounter(final BundleContext context) { + final ResourceResolverFactory resourceResolverFactory = proxy( + ResourceResolverFactory.class, + (method, args) -> "getSearchPath".equals(method.getName()) ? List.of("/apps/", "/libs/") : null); + final ResolverConfig config = proxy(ResolverConfig.class, (method, args) -> { + switch (method.getName()) { + case "servletresolver_mountProviders": + return true; // -> provider == null -> mountProviders() == true + case "servletresolver_mountPathProviders": + return false; + case "servletresolver_servletRoot": + return "0"; + default: + return null; + } + }); + return new ServletMounter(context, resourceResolverFactory, null, config); + } + + /** A DispatcherServlet registration as returned from the (stubbed) registration context. */ + private ServiceRegistration<Servlet> dispatcherRegistration() { + final ServiceReference<Servlet> ref = proxy(ServiceReference.class, (method, args) -> null); + return proxy(ServiceRegistration.class, (method, args) -> { + if ("unregister".equals(method.getName())) { + unregistrations.incrementAndGet(); + return null; + } + return "getReference".equals(method.getName()) ? ref : null; + }); + } + + /** An incoming servlet registration carrying a versioned resource type, as produced from a bundle. */ + private ServiceRegistration<Servlet> inputRegistration(final String versionedResourceType) { + final ServiceReference<Servlet> ref = proxy(ServiceReference.class, (method, args) -> { + if ("getProperty".equals(method.getName())) { + return ServletResolverConstants.SLING_SERVLET_RESOURCE_TYPES.equals(args[0]) + ? new String[] {versionedResourceType} + : null; + } + return "getBundle".equals(method.getName()) ? registeringBundle : null; + }); + return proxy(ServiceRegistration.class, (method, args) -> "getReference".equals(method.getName()) ? ref : null); + } + + /** Builds a {@link Proxy} for {@code iface}; {@code handler} answers all but the {@link Object} methods. */ + @SuppressWarnings("unchecked") + private static <T> T proxy(final Class<T> iface, final BiFunction<Method, Object[], Object> handler) { + return (T) Proxy.newProxyInstance(iface.getClassLoader(), new Class<?>[] {iface}, (p, method, args) -> { + switch (method.getName()) { + case "equals": + return p == args[0]; + case "hashCode": + return System.identityHashCode(p); + case "toString": + return iface.getSimpleName() + "@proxy"; + case "compareTo": + return 0; + default: + return handler.apply(method, args); + } + }); + } +}
