This is an automated email from the ASF dual-hosted git repository. royteeuwen pushed a commit to branch fix/resource-decorator-tracker-cleared-on-modified in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
commit 77e6e76b114c4326c610c780cde2283d8a6ce7af Author: Roy Teeuwen <[email protected]> AuthorDate: Wed May 20 13:35:00 2026 +0200 SLING-13206: ResourceDecorator services lost after @Modified on ResourceResolverFactoryActivator When OSGi fires the @Modified lifecycle callback (e.g. triggered by a bundle deployment causing a configuration change), modified() delegates to deactivateInternal() which calls resourceDecoratorTracker.close(), clearing all registered ResourceDecorator services. Because the decorators use ReferencePolicy.DYNAMIC, OSGi does not re-fire bindResourceDecorator after a @Modified event (no component restart). The tracker is therefore left permanently empty, silently breaking any feature that relies on ResourceDecorator (e.g. path-rewriting decorators). Fix: move resourceDecoratorTracker.close() out of deactivateInternal() and into deactivate() only. deactivateInternal() is still called from both @Modified and @Deactivate, but only the true component teardown path (@Deactivate) now closes the tracker. Also add a null-guard in ResourceDecoratorEntry.compareTo() to prevent NPE when ServiceReference is null. --- .../impl/ResourceResolverFactoryActivator.java | 7 +- .../impl/helper/ResourceDecoratorTracker.java | 3 + .../ResourceDecoratorTrackerLifecycleTest.java | 234 +++++++++++++++++++++ 3 files changed, 243 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java index f8bdaee4..b92af50a 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java @@ -369,6 +369,12 @@ public class ResourceResolverFactoryActivator { // factoryRegistrationHandler must be closed before bundleContext is set to null this.factoryRegistrationHandler.close(); this.bundleContext = null; + // Close the decorator tracker only on true deactivation. The tracker must NOT be + // closed inside deactivateInternal(), which is also called from @Modified. When + // @Modified fires, OSGi does not re-fire bindResourceDecorator for already-bound + // DYNAMIC references, so closing the tracker here would permanently lose all + // registered ResourceDecorators for the lifetime of the component. + this.resourceDecoratorTracker.close(); deactivateInternal(); } @@ -379,7 +385,6 @@ public class ResourceResolverFactoryActivator { this.changeListenerWhiteboard = null; this.resourceProviderTracker.deactivate(); this.resourceProviderTracker = null; - this.resourceDecoratorTracker.close(); } /** diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/helper/ResourceDecoratorTracker.java b/src/main/java/org/apache/sling/resourceresolver/impl/helper/ResourceDecoratorTracker.java index 3eea04a8..d600d73f 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/helper/ResourceDecoratorTracker.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/helper/ResourceDecoratorTracker.java @@ -141,6 +141,9 @@ public class ResourceDecoratorTracker { } public int compareTo(final ResourceDecoratorEntry o) { + if (this.comparable == null || o.comparable == null) { + return 0; + } return comparable.compareTo(o.comparable); } } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/ResourceDecoratorTrackerLifecycleTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceDecoratorTrackerLifecycleTest.java new file mode 100644 index 00000000..3124c0de --- /dev/null +++ b/src/test/java/org/apache/sling/resourceresolver/impl/ResourceDecoratorTrackerLifecycleTest.java @@ -0,0 +1,234 @@ +/* + * 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.resourceresolver.impl; + +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.List; + +import org.apache.sling.api.resource.ResourceDecorator; +import org.apache.sling.resourceresolver.impl.helper.ResourceDecoratorTracker; +import org.apache.sling.resourceresolver.impl.observation.ResourceChangeListenerWhiteboard; +import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; + +/** + * Regression test for the bug where {@link ResourceDecorator} services registered via OSGi + * are silently dropped from the {@link ResourceResolverFactoryActivator} after a configuration + * change triggers a {@code @Modified} lifecycle callback. + * + * <h2>Root cause</h2> + * {@code ResourceResolverFactoryActivator.modified()} calls {@code deactivateInternal()}, which + * in turn calls {@code resourceDecoratorTracker.close()}, clearing all registered decorators. + * Because the decorators were already bound (policy = DYNAMIC), OSGi does <em>not</em> re-fire + * the {@code bindResourceDecorator} callbacks after the modified callback completes. The tracker + * is therefore left permanently empty until the next bundle deployment cycle that happens to + * restart the affected decorator bundles. + * + * <p>This manifests in practice when deploying the AEM core bundle: its deployment can trigger a + * configuration change that causes {@code @Modified} on the activator, wiping out every + * {@code ResourceDecorator} (e.g. path-rewriting decorators) for the remainder of the instance + * lifetime. + * + * <h2>Fix</h2> + * {@code resourceDecoratorTracker.close()} is moved out of {@code deactivateInternal()} and + * into {@code deactivate()} only, so that it is called exclusively on true component teardown + * ({@code @Deactivate}). {@code deactivateInternal()} — which is still called by both + * {@code @Modified} and {@code @Deactivate} — no longer clears the tracker, leaving + * still-bound decorator services intact across configuration changes. + */ +public class ResourceDecoratorTrackerLifecycleTest { + + /** + * Simulates what happens when OSGi calls {@code @Modified} on the activator (e.g. because a + * bundle deployment causes a configuration change) while one or more {@link ResourceDecorator} + * services are already bound. + * + * <p>Before the fix this test fails because {@code deactivateInternal()} clears the tracker + * and OSGi never re-fires the dynamic bind callback. + */ + @Test + public void decoratorsArePreservedAfterModifiedLifecycleCallback() throws Exception { + ResourceResolverFactoryActivator activator = new ResourceResolverFactoryActivator(); + + // Pre-wire the package-private collaborators with mocks so that deactivateInternal() + // can execute without needing a live OSGi framework (it calls deactivate() on both). + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + + // Bind a decorator — this is what OSGi does when a ResourceDecorator service appears. + ResourceDecorator decorator = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decorator, null); + + assertEquals( + "Decorator must be registered before modified() is called", + 1, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + + // Invoke deactivateInternal() via reflection — this is the + // exact method that modified() now delegates to. We cannot call modified() directly + // because it also calls activate(), which requires a live BundleContext. + Method deactivateForModified = ResourceResolverFactoryActivator.class.getDeclaredMethod("deactivateInternal"); + deactivateForModified.setAccessible(true); + deactivateForModified.invoke(activator); + + // After deactivateInternal() the OSGi framework will NOT re-fire bindResourceDecorator + // for services that are still registered, because the reference policy is DYNAMIC and + // the component was not fully restarted. The decorator must therefore still be present. + assertEquals( + "Decorator must still be registered after deactivateInternal() — OSGi will not re-bind it", + 1, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + } + + /** + * Multiple consecutive {@code @Modified} events (e.g. several rapid configuration pushes) + * must not erode the decorator count. Each {@code deactivateInternal()} call must leave the + * tracker untouched. + */ + @Test + public void decoratorsArePreservedAcrossMultipleModifiedCycles() throws Exception { + ResourceResolverFactoryActivator activator = new ResourceResolverFactoryActivator(); + ResourceDecorator decorator = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decorator, null); + + Method deactivateForModified = ResourceResolverFactoryActivator.class.getDeclaredMethod("deactivateInternal"); + deactivateForModified.setAccessible(true); + + for (int cycle = 1; cycle <= 3; cycle++) { + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + deactivateForModified.invoke(activator); + + assertEquals( + "Decorator must survive modified() cycle #" + cycle, + 1, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + } + } + + /** + * After a {@code @Modified} event, a decorator whose bundle subsequently stops must still be + * correctly removed via {@code unbindResourceDecorator()}. The fix must not interfere with + * normal dynamic unbinding. + */ + @Test + public void decoratorCanBeUnboundAfterModifiedLifecycleCallback() throws Exception { + ResourceResolverFactoryActivator activator = new ResourceResolverFactoryActivator(); + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + + ResourceDecorator decorator = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decorator, null); + assertEquals(1, registeredDecoratorCount(activator.getResourceDecoratorTracker())); + + // Simulate @Modified — decorator must survive + Method deactivateForModified = ResourceResolverFactoryActivator.class.getDeclaredMethod("deactivateInternal"); + deactivateForModified.setAccessible(true); + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + deactivateForModified.invoke(activator); + + assertEquals( + "Decorator must still be present immediately after modified()", + 1, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + + // Now the decorator's bundle stops — OSGi calls unbindResourceDecorator + activator.unbindResourceDecorator(decorator); + + assertEquals( + "Decorator must be removed after explicit unbind following modified()", + 0, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + } + + /** + * A new {@link ResourceDecorator} that appears after a {@code @Modified} event (e.g. a + * freshly deployed bundle) must be accepted and tracked normally. + */ + @Test + public void newDecoratorCanBeAddedAfterModifiedLifecycleCallback() throws Exception { + ResourceResolverFactoryActivator activator = new ResourceResolverFactoryActivator(); + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + + ResourceDecorator decoratorA = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decoratorA, null); + + // Simulate @Modified + Method deactivateForModified = ResourceResolverFactoryActivator.class.getDeclaredMethod("deactivateInternal"); + deactivateForModified.setAccessible(true); + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + deactivateForModified.invoke(activator); + + // A second decorator registers after the config change (e.g. its bundle deployed later) + ResourceDecorator decoratorB = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decoratorB, null); + + assertEquals( + "Both the pre-existing and the newly added decorator must be tracked", + 2, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + } + + /** + * Sanity check: a true component deactivation (via {@code @Deactivate}) must still clear the + * tracker, because on the next component activation OSGi will re-bind all services. + */ + @Test + public void decoratorsAreClearedOnTrueDeactivation() throws Exception { + ResourceResolverFactoryActivator activator = new ResourceResolverFactoryActivator(); + + activator.changeListenerWhiteboard = mock(ResourceChangeListenerWhiteboard.class); + activator.resourceProviderTracker = mock(ResourceProviderTracker.class); + + ResourceDecorator decorator = mock(ResourceDecorator.class); + activator.bindResourceDecorator(decorator, null); + + assertEquals(1, registeredDecoratorCount(activator.getResourceDecoratorTracker())); + + activator.getResourceDecoratorTracker().close(); + + assertEquals( + "Tracker must be empty after a true component deactivation", + 0, + registeredDecoratorCount(activator.getResourceDecoratorTracker())); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + /** + * Returns the number of entries in {@link ResourceDecoratorTracker#resourceDecorators}. + * The field is {@code protected} in a sub-package, so we access it via reflection to avoid + * widening its visibility in production code. + */ + @SuppressWarnings("unchecked") + private static int registeredDecoratorCount(ResourceDecoratorTracker tracker) throws Exception { + Field field = ResourceDecoratorTracker.class.getDeclaredField("resourceDecorators"); + field.setAccessible(true); + return ((List<?>) field.get(tracker)).size(); + } +}
