This is an automated email from the ASF dual-hosted git repository. joerghoh pushed a commit to branch SLING-13393 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-i18n.git
commit fd89888d0cde64956c1dbc7b816037299a1cde18 Author: Joerg Hoh <[email protected]> AuthorDate: Wed Oct 14 17:43:43 2026 +0200 SLING-13393: Coordinate resource bundle publication with registry closure Co-authored-by: Copilot <[email protected]> --- .../sling/i18n/impl/JcrResourceBundleProvider.java | 52 +++++-- .../ConcurrentJcrResourceBundleLoadingTest.java | 172 ++++++++++++++++++++- 2 files changed, 199 insertions(+), 25 deletions(-) diff --git a/src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java b/src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java index 97047ab..f347869 100644 --- a/src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java +++ b/src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java @@ -889,6 +889,7 @@ public class JcrResourceBundleProvider * Registry of the loaded <code>resource bundles</code> and the associated <code>service registrations</code> * The <code>ResourceBundleRegistry</code> takes care of the registration/deregistration of the resource bundles as OSGi services. * It stores the references to the registered resource bundles and to the associated service registrations. + * Publication and removal are coordinated with closure; OSGi registration callbacks run outside the registry monitor. */ private static class ResourceBundleRegistry implements AutoCloseable { private final Logger log = LoggerFactory.getLogger(getClass()); @@ -928,7 +929,17 @@ public class JcrResourceBundleProvider } ServiceRegistration<ResourceBundle> serviceReg = bundleContext.registerService(ResourceBundle.class, resourceBundle, serviceProps(key)); - Entry oldEntry = registrations.get().put(key, new Entry(resourceBundle, serviceReg)); + final Entry oldEntry; + final boolean published; + synchronized (this) { + published = !closed.get(); + oldEntry = published ? registrations.get().put(key, new Entry(resourceBundle, serviceReg)) : null; + } + // Registration callbacks can close the registry before publication. + if (!published) { + serviceReg.unregister(); + return; + } if (oldEntry != null) { oldEntry.serviceRegistration.unregister(); } @@ -948,10 +959,13 @@ public class JcrResourceBundleProvider } void unregisterResourceBundle(Key key) { - if (closed.get()) { - return; + final Entry oldEntry; + synchronized (this) { + if (closed.get()) { + return; + } + oldEntry = registrations.get().remove(key); } - Entry oldEntry = registrations.get().remove(key); if (oldEntry != null) { oldEntry.serviceRegistration.unregister(); } else { @@ -962,18 +976,24 @@ public class JcrResourceBundleProvider } void unregisterAll() { - if (closed.get()) { - return; - } - unregisterAllInternal(); + unregisterAllInternal(false); } - private void unregisterAllInternal() { - log.debug( - "[ResourceBundleRegistry.clearInternal] Before - Nr of Keys: {} - Keys: {}", - registrations.get().size(), - registrations.get().keySet()); - ConcurrentHashMap<Key, Entry> oldServiceReg = registrations.getAndSet(new ConcurrentHashMap<>()); + private void unregisterAllInternal(boolean closing) { + final ConcurrentHashMap<Key, Entry> oldServiceReg; + synchronized (this) { + if (closed.get()) { + return; + } + if (closing) { + closed.set(true); + } + log.debug( + "[ResourceBundleRegistry.clearInternal] Before - Nr of Keys: {} - Keys: {}", + registrations.get().size(), + registrations.get().keySet()); + oldServiceReg = registrations.getAndSet(new ConcurrentHashMap<>()); + } for (Entry entry : oldServiceReg.values()) { entry.serviceRegistration.unregister(); } @@ -989,9 +1009,7 @@ public class JcrResourceBundleProvider @Override public void close() { - if (closed.compareAndSet(false, true)) { - unregisterAllInternal(); - } + unregisterAllInternal(true); } } } diff --git a/src/test/java/org/apache/sling/i18n/impl/ConcurrentJcrResourceBundleLoadingTest.java b/src/test/java/org/apache/sling/i18n/impl/ConcurrentJcrResourceBundleLoadingTest.java index bb8b670..90fba1a 100644 --- a/src/test/java/org/apache/sling/i18n/impl/ConcurrentJcrResourceBundleLoadingTest.java +++ b/src/test/java/org/apache/sling/i18n/impl/ConcurrentJcrResourceBundleLoadingTest.java @@ -20,6 +20,7 @@ package org.apache.sling.i18n.impl; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -32,6 +33,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -51,12 +53,15 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.mockito.Mockito; import org.osgi.framework.Constants; +import org.osgi.framework.ServiceEvent; +import org.osgi.framework.ServiceListener; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -79,6 +84,7 @@ public class ConcurrentJcrResourceBundleLoadingTest { public Boolean preload = Boolean.FALSE; private JcrResourceBundleProvider provider; + private ResourceResolverFactory resolverFactory; private final List<Resource> dictionaryRoots = new ArrayList<>(); @Before @@ -88,7 +94,7 @@ public class ConcurrentJcrResourceBundleLoadingTest { Mockito.doAnswer(query -> dictionaryRoots.iterator()) .when(resolver) .findResources(JcrResourceBundle.QUERY_LANGUAGE_ROOTS, "xpath"); - ResourceResolverFactory resolverFactory = Mockito.mock(ResourceResolverFactory.class); + resolverFactory = Mockito.mock(ResourceResolverFactory.class); Mockito.when(resolverFactory.getServiceResourceResolver(null)).thenReturn(resolver); context.registerService(ResourceResolverFactory.class, resolverFactory, Constants.SERVICE_RANKING, 1000); // mock other required services @@ -338,9 +344,149 @@ public class ConcurrentJcrResourceBundleLoadingTest { * Verify that no exception occurs if requests come in during deactivate */ @Test - public void loadBundlesDuringDeactivateRace() { - provider.deactivate(); - assertNotNull(provider.getResourceBundle(Locale.ENGLISH)); + public void loadBundlesDuringDeactivateRace() throws Exception { + CountDownLatch loading = new CountDownLatch(1); + CountDownLatch resume = new CountDownLatch(1); + ResourceResolver resolver = resolverFactory.getServiceResourceResolver(null); + Mockito.when(resolverFactory.getServiceResourceResolver(null)).thenAnswer(invocation -> { + loading.countDown(); + await(resume); + return resolver; + }); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future<ResourceBundle> lookup = executor.submit(() -> provider.getResourceBundle(Locale.ENGLISH)); + assertTrue(loading.await(10, TimeUnit.SECONDS)); + provider.deactivate(); + assertNoResourceBundleServices(); + resume.countDown(); + assertNotNull(lookup.get(10, TimeUnit.SECONDS)); + assertNotNull(provider.getResourceBundle(Locale.ENGLISH)); + assertNoResourceBundleServices(); + } finally { + resume.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS)); + } + } + + @Test + public void registrationDuringDeactivateDoesNotLeaveServices() throws Exception { + assertRegistrationDuringDeactivate(false); + } + + @Test + public void replacementDuringDeactivateDoesNotLeaveServices() throws Exception { + assertRegistrationDuringDeactivate(true); + } + + private void assertRegistrationDuringDeactivate(boolean replacement) throws Exception { + if (replacement) { + provider.getResourceBundle(Locale.ENGLISH); + } + CountDownLatch registering = new CountDownLatch(1); + CountDownLatch resume = new CountDownLatch(1); + Map<Long, Integer> unregistrations = new ConcurrentHashMap<>(); + ServiceListener listener = event -> { + Long serviceId = (Long) event.getServiceReference().getProperty(Constants.SERVICE_ID); + if (event.getType() == ServiceEvent.REGISTERED) { + registering.countDown(); + await(resume); + } else if (event.getType() == ServiceEvent.UNREGISTERING) { + unregistrations.merge(serviceId, 1, Integer::sum); + } + }; + context.bundleContext().addServiceListener(listener, "(&(objectClass=java.util.ResourceBundle)(locale=en))"); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future<ResourceBundle> lookup = executor.submit(() -> { + if (replacement) { + provider.reloadBundle(new Key(null, Locale.ENGLISH)); + } + return provider.getResourceBundle(Locale.ENGLISH); + }); + assertTrue(registering.await(10, TimeUnit.SECONDS)); + provider.deactivate(); + resume.countDown(); + assertNotNull(lookup.get(10, TimeUnit.SECONDS)); + assertNoResourceBundleServices(); + assertEquals(replacement ? 2 : 1, unregistrations.size()); + unregistrations.values().forEach(count -> assertEquals(Integer.valueOf(1), count)); + } finally { + resume.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS)); + context.bundleContext().removeServiceListener(listener); + } + } + + @Test + public void deactivateFromRegistrationCallbackDoesNotLeaveServices() throws Exception { + ServiceListener listener = event -> { + if (event.getType() == ServiceEvent.REGISTERED) { + provider.deactivate(); + } + }; + context.bundleContext().addServiceListener(listener, "(objectClass=java.util.ResourceBundle)"); + try { + assertNotNull(provider.getResourceBundle(Locale.ENGLISH)); + assertNoResourceBundleServices(); + } finally { + context.bundleContext().removeServiceListener(listener); + } + } + + @Test + public void replacementRetirementDuringDeactivateUnregistersEachServiceOnce() throws Exception { + provider.getResourceBundle(Locale.ENGLISH); + Long originalId = (Long) context.bundleContext() + .getServiceReference(ResourceBundle.class) + .getProperty(Constants.SERVICE_ID); + CountDownLatch retiring = new CountDownLatch(1); + CountDownLatch resume = new CountDownLatch(1); + Map<Long, Integer> unregistrations = new ConcurrentHashMap<>(); + ServiceListener listener = event -> { + if (event.getType() == ServiceEvent.UNREGISTERING) { + Long serviceId = (Long) event.getServiceReference().getProperty(Constants.SERVICE_ID); + unregistrations.merge(serviceId, 1, Integer::sum); + if (originalId.equals(serviceId)) { + retiring.countDown(); + await(resume); + } + } + }; + context.bundleContext().addServiceListener(listener, "(objectClass=java.util.ResourceBundle)"); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future<?> reload = executor.submit(() -> provider.reloadBundle(new Key(null, Locale.ENGLISH))); + assertTrue(retiring.await(10, TimeUnit.SECONDS)); + provider.deactivate(); + resume.countDown(); + reload.get(10, TimeUnit.SECONDS); + assertNoResourceBundleServices(); + assertEquals(preload ? 2 : 1, unregistrations.size()); + unregistrations.values().forEach(count -> assertEquals(Integer.valueOf(1), count)); + } finally { + resume.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(10, TimeUnit.SECONDS)); + context.bundleContext().removeServiceListener(listener); + } + } + + private void assertNoResourceBundleServices() throws Exception { + assertTrue(context.bundleContext() + .getServiceReferences(ResourceBundle.class, null) + .isEmpty()); + } + + private static void await(CountDownLatch latch) { + try { + assertTrue(latch.await(10, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError("Interrupted while waiting for the test gate", e); + } } /** @@ -366,15 +512,17 @@ public class ConcurrentJcrResourceBundleLoadingTest { // Use a barrier to start the execution of all the threads simultaneously once they are all ready final CyclicBarrier barrier = new CyclicBarrier(numberOfThreads); final ExecutorService pool = Executors.newFixedThreadPool(numberOfThreads); + final List<Future<?>> tasks = new ArrayList<>(); for (int i = 0; i < numberOfThreads; i++) { - pool.submit(new Runnable() { + tasks.add(pool.submit(new Runnable() { @Override public void run() { try { barrier.await(); for (Locale locale : testLocales) { ResourceBundle rb = provider.getResourceBundle(locale); - List<ResourceBundle> rbList = rbLists.computeIfAbsent(locale, key -> new ArrayList<>()); + List<ResourceBundle> rbList = rbLists.computeIfAbsent( + locale, key -> Collections.synchronizedList(new ArrayList<>())); rbList.add(rb); } // trigger registry clearing and service deregistration @@ -383,10 +531,18 @@ public class ConcurrentJcrResourceBundleLoadingTest { throw new RuntimeException(e); } } - }); + })); } pool.shutdown(); - pool.awaitTermination(5, TimeUnit.SECONDS); + try { + for (Future<?> task : tasks) { + task.get(10, TimeUnit.SECONDS); + } + assertNoResourceBundleServices(); + } finally { + pool.shutdownNow(); + assertTrue(pool.awaitTermination(10, TimeUnit.SECONDS)); + } // getResourceBundle should return a new ResourceBundle instance, // not one that might be left in the cache by an incomplete clearing
