diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index bfda1ce5f..10c27101b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -68,7 +68,14 @@ jobs: fi unzip openidm-zip/target/openidm-*.zip openidm/startup.sh & - timeout 3m bash -c 'until grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 ; do sleep 5; done' || cat openidm/logs/openidm0.log.0 + timeout 3m bash -c 'until grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 ; do sleep 5; done' || { + cat openidm/logs/openidm0.log.0 + # A start-up that never gets ready is a hang (#228): dump the threads before failing. + for pid in $(pgrep -f org.forgerock.commons.launcher.Main); do + jcmd "$pid" Thread.print -l > openidm/logs/threaddump-$pid.txt 2>&1 || true + cat openidm/logs/threaddump-$pid.txt + done + } grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 ! grep -E "ERROR|SEVERE|Exception|Throwable" openidm/logs/openidm0.log.0 - name: Test on Windows @@ -185,7 +192,14 @@ jobs: ARGS="-p ${{ matrix.samples }}" fi OPENIDM_OPTS="$OPTS" openidm/startup.sh $ARGS & - timeout 3m bash -c 'until grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 ; do sleep 5; done' || cat openidm/logs/openidm0.log.0 + timeout 3m bash -c 'until grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 ; do sleep 5; done' || { + cat openidm/logs/openidm0.log.0 + # A start-up that never gets ready is a hang (#228): dump the threads before failing. + for pid in $(pgrep -f org.forgerock.commons.launcher.Main); do + jcmd "$pid" Thread.print -l > openidm/logs/threaddump-$pid.txt 2>&1 || true + cat openidm/logs/threaddump-$pid.txt + done + } grep -q "OpenIDM ready" openidm/logs/openidm0.log.0 # Allow-list of documented, expected log-noise per sample. The # usecase1 walk-through explicitly relies on three iterative recon diff --git a/openidm-servlet-registrator/src/main/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingleton.java b/openidm-servlet-registrator/src/main/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingleton.java index 6fcdecc6f..730963746 100644 --- a/openidm-servlet-registrator/src/main/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingleton.java +++ b/openidm-servlet-registrator/src/main/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingleton.java @@ -43,6 +43,7 @@ import java.util.Hashtable; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import jakarta.servlet.Filter; import jakarta.servlet.Servlet; @@ -113,7 +114,15 @@ private static String[] getDefaultServletUrlPatterns() { @Reference private WebContainer webContainer; - private HttpContext sharedContext; + /** + * Created on first use rather than in {@link #activate}: SCR holds this component's state lock + * during activation, and a {@link WebContainer} call from any thread other than the pax-web + * configuration thread waits for that thread. The configuration thread in turn waits for this + * state lock when it registers the {@link WebContainer} service and activates the components + * that depend on {@link ServletRegistration}. No lock is held while the context is created, for + * the same reason; a concurrently created duplicate is discarded. + */ + private final AtomicReference sharedContext = new AtomicReference<>(); private List filters = new ArrayList(); @@ -127,7 +136,15 @@ private static String[] getDefaultServletUrlPatterns() { @Activate public void activate(ComponentContext context) { bundleContext = context.getBundleContext(); - sharedContext = webContainer.createDefaultSharedHttpContext(); + } + + private HttpContext sharedContext() { + HttpContext context = sharedContext.get(); + if (context == null) { + sharedContext.compareAndSet(null, webContainer.createDefaultSharedHttpContext()); + context = sharedContext.get(); + } + return context; } /** @@ -145,7 +162,7 @@ public void deactivate(ComponentContext context) { */ @SuppressWarnings("rawtypes") public void registerServlet(String alias, Servlet servlet, Dictionary initparams) throws ServletException, NamespaceException { - webContainer.registerServlet(alias, servlet, initparams, sharedContext); + webContainer.registerServlet(alias, servlet, initparams, sharedContext()); } /** @@ -157,7 +174,7 @@ public void unregisterServlet(Servlet servlet) { @Override public HttpContext getContext() { - return sharedContext; + return sharedContext(); } /** @@ -263,7 +280,7 @@ public URL apply(JsonValue jsonValue) throws JsonValueException { urlPatterns.toArray(new String[urlPatterns.size()]), servletNames.toArray(new String[servletNames.size()]), new Hashtable<>(initParams), - sharedContext); + sharedContext()); return proxiedFilter; } diff --git a/openidm-servlet-registrator/src/test/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingletonTest.java b/openidm-servlet-registrator/src/test/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingletonTest.java new file mode 100644 index 000000000..5fe4384cc --- /dev/null +++ b/openidm-servlet-registrator/src/test/java/org/forgerock/openidm/servletregistration/impl/ServletRegistrationSingletonTest.java @@ -0,0 +1,86 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ + +package org.forgerock.openidm.servletregistration.impl; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertSame; + +import java.lang.reflect.Field; +import java.util.Hashtable; + +import jakarta.servlet.Servlet; + +import org.ops4j.pax.web.service.MultiBundleWebContainerContext; +import org.ops4j.pax.web.service.WebContainer; +import org.osgi.framework.BundleContext; +import org.osgi.service.component.ComponentContext; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +public class ServletRegistrationSingletonTest { + + private WebContainer webContainer; + private MultiBundleWebContainerContext sharedContext; + private ServletRegistrationSingleton registration; + + @BeforeMethod + public void setUp() throws Exception { + webContainer = mock(WebContainer.class); + sharedContext = mock(MultiBundleWebContainerContext.class); + when(webContainer.createDefaultSharedHttpContext()).thenReturn(sharedContext); + + registration = new ServletRegistrationSingleton(); + Field field = ServletRegistrationSingleton.class.getDeclaredField("webContainer"); + field.setAccessible(true); + field.set(registration, webContainer); + } + + /** + * SCR holds the component's state lock while activate() runs. A WebContainer call made there is + * executed on the single pax-web configuration thread, which may itself be waiting for that + * state lock while it registers the WebContainer service (#228), so activate() must not call it. + */ + @Test + public void activateDoesNotCallWebContainer() { + registration.activate(componentContext()); + + verifyZeroInteractions(webContainer); + } + + @Test + public void sharedContextIsCreatedOnceOnFirstUse() throws Exception { + registration.activate(componentContext()); + Servlet servlet = mock(Servlet.class); + Hashtable params = new Hashtable<>(); + + assertSame(registration.getContext(), sharedContext); + registration.registerServlet("/openidm", servlet, params); + + verify(webContainer, times(1)).createDefaultSharedHttpContext(); + verify(webContainer).registerServlet("/openidm", servlet, params, sharedContext); + } + + private static ComponentContext componentContext() { + ComponentContext context = mock(ComponentContext.class); + when(context.getBundleContext()).thenReturn(mock(BundleContext.class)); + return context; + } +}