This is an automated email from the ASF dual-hosted git repository.
reschke pushed a commit to branch SLING-13236
in repository
https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
The following commit(s) were added to refs/heads/SLING-13236 by this push:
new 4e61eaa2 SLING-13236 - MapEntries: NPE when resource change is
delivered during construction or disposal (#210)
4e61eaa2 is described below
commit 4e61eaa231878eea513104dfd7ebda82804a0423
Author: Herman Ciechanowiec <[email protected]>
AuthorDate: Fri Jul 10 12:48:45 2026 +0200
SLING-13236 - MapEntries: NPE when resource change is delivered during
construction or disposal (#210)
* SLING-13236 - MapEntries: NPE when resource change is delivered during
construction or disposal
The MapEntries constructor registered itself as a ResourceChangeListener
before assigning the VanityPathHandler field, so a change delivered by the
asynchronous "Apache Sling Resource Provider Change Notifier" thread in
that window caused:
java.lang.NullPointerException: Cannot invoke
"...VanityPathHandler.isReady()" because "this.vph" is null
at ...MapEntries.onChange(MapEntries.java:460)
Symmetrically, dispose() tore down the AliasHandler before unregistering
the listener, allowing the equivalent NPE on "this.ah" during shutdown.
Fix: assign the VanityPathHandler before registering the listener (events
arriving before initializeVanityPaths completes are queued and drained by
the initializer, as designed), and unregister the listener first in
dispose(). Add deterministic regression tests for both races.
* Update
src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
Co-authored-by: Jörg Hoh <[email protected]>
* Update
src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
Co-authored-by: Jörg Hoh <[email protected]>
* SLING-13236 - Apply spotless formatting to MapEntriesTest
---------
Co-authored-by: Jörg Hoh <[email protected]>
---
.../resourceresolver/impl/mapping/MapEntries.java | 22 ++++++---
.../impl/mapping/MapEntriesTest.java | 54 ++++++++++++++++++++++
2 files changed, 69 insertions(+), 7 deletions(-)
diff --git
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
index 55b06869..1784481b 100644
---
a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
+++
b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
@@ -142,10 +142,15 @@ public class MapEntries implements MapEntriesHandler,
ResourceChangeListener, Ex
this::drainAliasQueue);
this.ah.initializeAliases();
- this.registration = registerResourceChangeListener(bundleContext);
-
+ // create the vanity path handler before registering the resource
change listener: the
+ // listener may be called as soon as it is registered, and onChange
requires both handlers
+ // to be present (events arriving before initializeVanityPaths
completes are queued and
+ // drained by the initializer)
this.vph =
new VanityPathHandler(this.factory, this.resolveMapsMap,
this.initializing, this::drainVanityPathQueue);
+
+ this.registration = registerResourceChangeListener(bundleContext);
+
this.vph.initializeVanityPaths();
if (metrics.isPresent()) {
@@ -293,16 +298,19 @@ public class MapEntries implements MapEntriesHandler,
ResourceChangeListener, Ex
*/
public void dispose() {
- if (this.ah != null) {
- ah.dispose();
- ah = null;
- }
-
+ // unregister the resource change listener before disposing the alias
handler: onChange
+ // requires the handlers to be present, so they must not be torn down
while the listener
+ // can still be called
if (this.registration != null) {
this.registration.unregister();
this.registration = null;
}
+ if (this.ah != null) {
+ ah.dispose();
+ ah = null;
+ }
+
/*
* Cooperation with doInit: The same lock as used by doInit is acquired
* thus preventing doInit from running and waiting for a concurrent
diff --git
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
index 237c5109..387a3657 100644
---
a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
+++
b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
@@ -37,6 +37,8 @@ import java.util.concurrent.atomic.AtomicInteger;
import org.apache.commons.lang3.StringUtils;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
+import org.apache.sling.api.resource.observation.ResourceChange;
+import org.apache.sling.api.resource.observation.ResourceChangeListener;
import org.apache.sling.api.resource.path.Path;
import org.apache.sling.resourceresolver.impl.ResourceResolverMetrics;
import org.junit.After;
@@ -48,6 +50,7 @@ import org.mockito.MockitoAnnotations;
import org.mockito.stubbing.Answer;
import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
+import org.osgi.framework.ServiceRegistration;
import org.osgi.service.event.EventAdmin;
import static org.junit.Assert.assertEquals;
@@ -223,6 +226,57 @@ public class MapEntriesTest extends
AbstractMappingMapEntriesTest {
mapEntries.ah.initializeAliases();
}
+ // tests SLING-13236: a resource change delivered while the MapEntries
constructor is still
+ // running (the listener may be called as soon as it is registered) must
not cause an NPE on
+ // the not-yet-assigned VanityPathHandler
+ @Test
+ public void testChangeDeliveredDuringListenerRegistration() throws
Exception {
+ final AtomicBoolean changeDelivered = new AtomicBoolean(false);
+ when(bundleContext.registerService(eq(ResourceChangeListener.class),
any(ResourceChangeListener.class), any()))
+
.thenAnswer((Answer<ServiceRegistration<ResourceChangeListener>>) invocation ->
{
+ final ResourceChangeListener listener =
invocation.getArgument(1);
+ listener.onChange(List.of(new
ResourceChange(ResourceChange.ChangeType.ADDED, "/resource", false)));
+ changeDelivered.set(true);
+ return null;
+ });
+
+ final MapEntries entries = new MapEntries(
+ resourceResolverFactory, bundleContext, eventAdmin,
stringInterpolationProvider, Optional.empty());
+ try {
+ assertTrue("change should have been delivered during listener
registration", changeDelivered.get());
+ } finally {
+ entries.dispose();
+ }
+ }
+
+ // tests SLING-13236: a resource change delivered while dispose() is
unregistering the
+ // listener must not cause an NPE on an already torn down AliasHandler
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testChangeDeliveredDuringDispose() throws Exception {
+ final AtomicBoolean changeDelivered = new AtomicBoolean(false);
+ when(bundleContext.registerService(eq(ResourceChangeListener.class),
any(ResourceChangeListener.class), any()))
+
.thenAnswer((Answer<ServiceRegistration<ResourceChangeListener>>) invocation ->
{
+ final ResourceChangeListener listener =
invocation.getArgument(1);
+ final ServiceRegistration<ResourceChangeListener>
registration =
+ Mockito.mock(ServiceRegistration.class);
+ Mockito.doAnswer(unregisterInvocation -> {
+ listener.onChange(List.of(
+ new
ResourceChange(ResourceChange.ChangeType.ADDED, "/resource", false)));
+ changeDelivered.set(true);
+ return null;
+ })
+ .when(registration)
+ .unregister();
+ return registration;
+ });
+
+ final MapEntries entries = new MapEntries(
+ resourceResolverFactory, bundleContext, eventAdmin,
stringInterpolationProvider, Optional.empty());
+ entries.dispose();
+ assertTrue("change should have been delivered during listener
unregistration", changeDelivered.get());
+ }
+
@Test
public void testTimingFormatter() {
assertEquals(
