diff --git a/opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java b/opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java index 27581c4356..482b3188be 100644 --- a/opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java +++ b/opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java @@ -190,6 +190,13 @@ public void run() { */ private final Object waitListen = new Object(); + /** + * Condition predicate for {@link #waitListen}: set once the handler thread has attempted to open the listen socket + * (successfully or not). Guarded by the {@link #waitListen} monitor; protects the start method against spurious + * wakeups. + */ + private boolean listenAttempted; + /** The friendly name of this connection handler. */ private String friendlyName; @@ -679,6 +686,38 @@ public Stream handle(final LDAPClientContext context, logger.info(NOTE_CONNHANDLER_STARTED_LISTENING, handlerName); } + /** + * {@inheritDoc} + *

+ * Deliberately left unsynchronized, unlike the overridden {@link Thread#start()}: the state this method touches is + * guarded by the {@link #waitListen} monitor, and holding the thread's own monitor across {@code waitListen.wait()} + * would block {@link Thread#join()}. + */ + @Override + public void start() { + // The Directory Server start process should only return when the connection handler port is fully opened + // and working. The start method therefore needs to wait for the created thread. + synchronized (waitListen) { + super.start(); + + final long deadlineNanos = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(LISTEN_TIMEOUT_MS); + try { + while (!listenAttempted) { + final long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadlineNanos - System.nanoTime()); + if (remainingMs <= 0) { + logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, handlerName, + TimeUnit.MILLISECONDS.toSeconds(LISTEN_TIMEOUT_MS)); + break; + } + waitListen.wait(remainingMs); + } + } catch (InterruptedException e) { + // If something interrupted the start its probably better to return ASAP. + Thread.currentThread().interrupt(); + } + } + } + /** * Operates in a loop, accepting new connections and ensuring that requests on those connections are handled * properly. @@ -702,6 +741,7 @@ public void run() { // so notify here to allow the server startup to complete. synchronized (waitListen) { starting = false; + listenAttempted = true; waitListen.notifyAll(); } } @@ -717,12 +757,6 @@ public void run() { } try { - // At this point, the connection Handler either started correctly or failed - // to start but the start process should be notified and resume its work in any cases. - synchronized (waitListen) { - waitListen.notifyAll(); - } - // If we have gotten here, then we are about to start listening // for the first time since startup or since we were previously disabled. startListener(); @@ -750,6 +784,13 @@ public void run() { } else { lastIterationFailed = true; } + } finally { + // At this point, the connection handler either started listening or failed to do so, + // but the start process should be notified and resume its work in any cases. + synchronized (waitListen) { + listenAttempted = true; + waitListen.notifyAll(); + } } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/api/ConnectionHandler.java b/opendj-server-legacy/src/main/java/org/opends/server/api/ConnectionHandler.java index 6a33bb399b..3645b8ba75 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/api/ConnectionHandler.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/api/ConnectionHandler.java @@ -13,6 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2012-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.api; @@ -23,6 +24,7 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.concurrent.TimeUnit; import org.forgerock.i18n.LocalizableMessage; import org.forgerock.opendj.server.config.server.ConnectionHandlerCfg; @@ -53,6 +55,20 @@ public abstract class ConnectionHandler private static final LocalizedLogger logger = LocalizedLogger.getLoggerForThisClass(); + /** + * Maximum time a {@code start()} implementation waits for its handler thread + * to attempt to open the listen socket. The wait is bounded so that a handler + * thread dying before it reaches the listen code cannot hang the whole server + * startup. + *

+ * {@code DirectoryServer.startConnectionHandlers()} starts the handlers one + * after another, so the worst case for a start is this value multiplied by + * the number of configured handlers. It is therefore kept far below the + * {@code start-ds} timeout ({@code DirectoryServer.DEFAULT_TIMEOUT}, 200 + * seconds), which is generous for a {@code bind()}. + */ + protected static final long LISTEN_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(15); + /** The monitor associated with this connection handler. */ private ConnectionHandlerMonitor monitor; diff --git a/opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java b/opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java index af5b881f7f..7f9cce50e5 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java @@ -167,8 +167,10 @@ public class HTTPConnectionHandler extends ConnectionHandler + * Deliberately left unsynchronized, unlike the overridden {@link Thread#start()}: + * the state this method touches is guarded by the {@link #waitListen} monitor, and + * holding the thread's own monitor across {@code waitListen.wait()} would block + * {@link Thread#join()}. + */ @Override public void start() { @@ -584,11 +594,19 @@ public void start() { super.start(); + final long deadlineNanos = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(LISTEN_TIMEOUT_MS); try { while (!listenAttempted) { - waitListen.wait(); + final long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadlineNanos - System.nanoTime()); + if (remainingMs <= 0) + { + logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, handlerName, + TimeUnit.MILLISECONDS.toSeconds(LISTEN_TIMEOUT_MS)); + break; + } + waitListen.wait(remainingMs); } } catch (InterruptedException e) @@ -654,14 +672,6 @@ public void run() try { - // At this point, the connection Handler either started correctly or failed - // to start but the start process should be notified and resume its work in any cases. - synchronized (waitListen) - { - listenAttempted = true; - waitListen.notifyAll(); - } - // If we have gotten here, then we are about to start listening // for the first time since startup or since we were previously disabled. // Start the embedded HTTP server @@ -695,6 +705,16 @@ public void run() lastIterationFailed = true; } } + finally + { + // At this point, the connection handler either started correctly or failed to start + // but the start process should be notified and resume its work in any cases. + synchronized (waitListen) + { + listenAttempted = true; + waitListen.notifyAll(); + } + } } // Initiate shutdown diff --git a/opendj-server-legacy/src/messages/org/opends/messages/protocol.properties b/opendj-server-legacy/src/messages/org/opends/messages/protocol.properties index 745726dbf0..60dccc8dc4 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/protocol.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/protocol.properties @@ -729,6 +729,10 @@ ERR_LDAPV2_CONTROLS_NOT_ALLOWED_431=LDAPv2 clients are not allowed to \ use request controls ERR_CONNHANDLER_CANNOT_BIND_432=The %s connection handler \ defined in configuration entry %s was unable to bind to %s:%d: %s +ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER_1540=The %s did not attempt to \ + open its listen port within %d seconds. The Directory Server will stop \ + waiting for it and continue, but that connection handler may never accept \ + connections ERR_JMX_SEARCH_INSUFFICIENT_PRIVILEGES_438=You do not have sufficient \ privileges to perform search operations through JMX ERR_JMX_INSUFFICIENT_PRIVILEGES_439=You do not have sufficient \ diff --git a/opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java b/opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java index 9f220fa7e8..fdd3d63787 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/TestCaseUtils.java @@ -51,6 +51,7 @@ import java.lang.management.ThreadInfo; import java.lang.management.ThreadMXBean; import java.net.BindException; +import java.net.ConnectException; import java.net.InetSocketAddress; import java.net.ServerSocket; import java.net.Socket; @@ -841,6 +842,62 @@ public static int[] findFreePorts(int nb) throws IOException } } + /** + * Asserts that the given local port accepts connections right now, without any retry loop. + *

+ * Intended for connection handler tests: a handler start method must not return before its listen + * port is open. When the port is closed this method tells the two possible causes apart, because a + * plain connection failure cannot distinguish them: the handler may never have managed to bind at + * all, or its start method may have returned too early. + * + * @param port + * the port the connection handler under test is supposed to be listening on + * @throws IOException + * if the connection fails for a reason other than the port being closed + */ + public static void assertPortIsAcceptingConnections(int port) throws IOException + { + try + { + connectTo(port); + } + catch (ConnectException e) + { + // Give the handler thread the extra time it should not have needed. If the port opens now, the + // handler can bind and the start method simply returned too early, which is the regression this + // check is about. If it never opens, the test failed for an unrelated reason. + assertTrue(waitForPortToOpen(port), "the connection handler never opened port " + port); + fail("the connection handler start method returned before port " + port + " was open"); + } + } + + private static void connectTo(int port) throws IOException + { + try (Socket socket = new Socket()) + { + socket.connect(new InetSocketAddress("127.0.0.1", port), 1000); + } + } + + private static boolean waitForPortToOpen(int port) + { + final long deadline = System.currentTimeMillis() + 10000; + do + { + try + { + connectTo(port); + return true; + } + catch (IOException ignored) + { + sleep(100); + } + } + while (System.currentTimeMillis() < deadline); + return false; + } + /** * Finds a free server socket port on the local host. * diff --git a/opendj-server-legacy/src/test/java/org/opends/server/protocols/http/HTTPConnectionHandlerTestCase.java b/opendj-server-legacy/src/test/java/org/opends/server/protocols/http/HTTPConnectionHandlerTestCase.java new file mode 100644 index 0000000000..456284bf30 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/protocols/http/HTTPConnectionHandlerTestCase.java @@ -0,0 +1,97 @@ +/* + * 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.opends.server.protocols.http; + +import static org.testng.Assert.*; + +import org.forgerock.i18n.LocalizableMessage; +import org.forgerock.opendj.server.config.meta.HTTPConnectionHandlerCfgDefn; +import org.forgerock.opendj.server.config.server.HTTPConnectionHandlerCfg; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.core.DirectoryServer; +import org.opends.server.extensions.InitializationUtils; +import org.opends.server.types.Entry; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +@SuppressWarnings("javadoc") +@Test(groups = { "precommit", "http" }, sequential = true) +public class HTTPConnectionHandlerTestCase extends DirectoryServerTestCase +{ + private static final LocalizableMessage STOP_REASON = LocalizableMessage.raw("Don't need a reason."); + + @BeforeClass + public void setUp() throws Exception + { + // This test suite depends on having the schema available, so we'll start the server. + TestCaseUtils.startServer(); + } + + /** + * The start method must not return before the handler thread has attempted to start the embedded + * HTTP server, otherwise a client connecting right after the handler has been enabled through + * dsconfig can be refused. + * + * @throws Exception + * if the handler cannot be instantiated or started. + */ + @Test + public void testStartWaitsForListenPort() throws Exception + { + final int listenPort = TestCaseUtils.findFreePort(); + Entry handlerEntry = TestCaseUtils.makeEntry( + "dn: cn=HTTP Connection Handler,cn=Connection Handlers,cn=config", + "objectClass: top", + "objectClass: ds-cfg-connection-handler", + "objectClass: ds-cfg-http-connection-handler", + "cn: HTTP Connection Handler", + "ds-cfg-java-class: org.opends.server.protocols.http.HTTPConnectionHandler", + "ds-cfg-enabled: true", + "ds-cfg-listen-address: 127.0.0.1", + "ds-cfg-listen-port: " + listenPort, + "ds-cfg-accept-backlog: 128", + "ds-cfg-keep-stats: false", + "ds-cfg-use-tcp-keep-alive: true", + "ds-cfg-use-tcp-no-delay: true", + "ds-cfg-allow-tcp-reuse-address: true", + "ds-cfg-max-request-size: 5 megabytes", + "ds-cfg-buffer-size: 4096 bytes", + "ds-cfg-max-blocked-write-time-limit: 2 minutes", + "ds-cfg-use-ssl: false", + "ds-cfg-ssl-client-auth-policy: optional", + "ds-cfg-ssl-cert-nickname: server-cert"); + HTTPConnectionHandlerCfg config = + InitializationUtils.getConfiguration(HTTPConnectionHandlerCfgDefn.getInstance(), handlerEntry); + + HTTPConnectionHandler handler = new HTTPConnectionHandler(); + handler.initializeConnectionHandler(DirectoryServer.getInstance().getServerContext(), config); + try + { + handler.start(); + + // No retry loop here on purpose: once start() has returned, the port must already be open. + TestCaseUtils.assertPortIsAcceptingConnections(listenPort); + } + finally + { + handler.processServerShutdown(STOP_REASON); + handler.finalizeConnectionHandler(STOP_REASON); + handler.join(10000); + assertFalse(handler.isAlive(), "the connection handler thread is still running"); + } + } +} diff --git a/opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/TestLDAPConnectionHandler.java b/opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/TestLDAPConnectionHandler.java index c0a6467a8b..5d167452b7 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/TestLDAPConnectionHandler.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/TestLDAPConnectionHandler.java @@ -13,7 +13,7 @@ * * Copyright 2006-2009 Sun Microsystems, Inc. * Portions Copyright 2012-2016 ForgeRock AS. - * Portions Copyright 2025 3A Systems, LLC. + * Portions Copyright 2025-2026 3A Systems, LLC. */ package org.opends.server.protocols.ldap; @@ -126,6 +126,57 @@ public void testLDAPConnectionHandler() throws Exception { LDAPConnHandler.processServerShutdown(reasonMsg); } + /** + * The start method must not return before the handler thread has attempted to + * open the listen port, otherwise a client connecting right after the server + * startup can be refused. + * + * @throws Exception if the handler cannot be instantiated or started. + */ + @Test + public void testStartWaitsForListenPort() throws Exception { + Entry handlerEntry = TestCaseUtils.makeEntry( + "dn: cn=LDAP Connection Handler,cn=Connection Handlers,cn=config", + "objectClass: top", + "objectClass: ds-cfg-connection-handler", + "objectClass: ds-cfg-ldap-connection-handler", + "cn: LDAP Connection Handler", + "ds-cfg-java-class: org.forgerock.opendj.reactive.LDAPConnectionHandler2", + "ds-cfg-enabled: true", + "ds-cfg-listen-address: 127.0.0.1", + "ds-cfg-accept-backlog: 128", + "ds-cfg-allow-ldap-v2: false", + "ds-cfg-keep-stats: false", + "ds-cfg-use-tcp-keep-alive: true", + "ds-cfg-use-tcp-no-delay: true", + "ds-cfg-allow-tcp-reuse-address: true", + "ds-cfg-send-rejection-notice: true", + "ds-cfg-max-request-size: 5 megabytes", + "ds-cfg-num-request-handlers: 2", + "ds-cfg-allow-start-tls: false", + "ds-cfg-use-ssl: false", + "ds-cfg-ssl-client-auth-policy: optional", + "ds-cfg-ssl-cert-nickname: server-cert", + "ds-cfg-key-manager-provider: cn=JKS,cn=Key Manager Providers,cn=config", + "ds-cfg-trust-manager-provider: cn=JKS,cn=Trust Manager Providers,cn=config"); + LDAPConnectionHandler2 handler = getLDAPHandlerInstance(handlerEntry); + int listenPort = handler.getListeners().iterator().next().getPort(); + try + { + handler.start(); + + // No retry loop here on purpose: once start() has returned, the port must already be open. + TestCaseUtils.assertPortIsAcceptingConnections(listenPort); + } + finally + { + handler.processServerShutdown(reasonMsg); + handler.finalizeConnectionHandler(reasonMsg); + handler.join(10000); + assertFalse(handler.isAlive(), "the connection handler thread is still running"); + } + } + /** * Start a handler an then give its hasAcceptableConfiguration a Entry with * numerous invalid cases and single-valued attrs with duplicate values.