From de146f81363356af3cb5cce9eae9c8ddab379ce9 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Thu, 30 Jul 2026 16:55:38 +0300 Subject: [PATCH 1/2] [#794] Wait for the listen port to be open before returning from connection handler start LDAPConnectionHandler2 declared a waitListen monitor but never waited on it, so DirectoryServer.startServer() could return while the LDAP, LDAPS and administration ports were still closed: a client connecting right afterwards got "Connection refused". HTTPConnectionHandler did wait, but notified before starting the embedded server, so its handshake had the same hole plus a spurious-wakeup window. Both handlers now set a listenAttempted predicate under the waitListen monitor after the bind attempt (in a finally block, so a failed bind unblocks startup too), and start() waits on that predicate. The wait is bounded to 60 seconds and logs an error on expiry, so a handler thread dying before it reaches the listen code cannot hang server startup. Adds TestLDAPConnectionHandler.testStartWaitsForListenPort, which connects to the handler port immediately after start() with no retry loop; it fails with "Connection refused" without the fix. Fixes #794 --- .../reactive/LDAPConnectionHandler2.java | 52 +++++++++++++++-- .../protocols/http/HTTPConnectionHandler.java | 42 ++++++++++---- .../org/opends/messages/protocol.properties | 4 ++ .../ldap/TestLDAPConnectionHandler.java | 57 ++++++++++++++++++- 4 files changed, 137 insertions(+), 18 deletions(-) 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..f1f9afcdaa 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 @@ -133,6 +133,12 @@ public void run() { /** SSL instance name used in context creation. */ private static final String SSL_CONTEXT_INSTANCE_NAME = "TLS"; + /** + * Maximum time the start method waits for the 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. + */ + private static final long LISTEN_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(60); + private LDAPListener listener; /** The current configuration state. */ @@ -190,6 +196,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 +692,31 @@ public Stream handle(final LDAPClientContext context, logger.info(NOTE_CONNHANDLER_STARTED_LISTENING, handlerName); } + @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, friendlyName, currentConfig.dn(), + 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 +740,7 @@ public void run() { // so notify here to allow the server startup to complete. synchronized (waitListen) { starting = false; + listenAttempted = true; waitListen.notifyAll(); } } @@ -717,12 +756,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 +783,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/protocols/http/HTTPConnectionHandler.java b/opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java index af5b881f7f..4521c4a398 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 @@ -120,6 +120,14 @@ public class HTTPConnectionHandler extends ConnectionHandler Date: Fri, 31 Jul 2026 11:27:32 +0300 Subject: [PATCH 2/2] [#794] Address review feedback on the connection handler listen wait Adds HTTPConnectionHandlerTestCase.testStartWaitsForListenPort: HTTPConnectionHandler.start() had no coverage at all, and the change to it is a bug fix rather than a cleanup -- enabling the handler through dsconfig reported success while the port was still closed. The test fails with "the connection handler start method returned before port N was open" against the previous HTTPConnectionHandler. Moves LISTEN_TIMEOUT_MS to ConnectionHandler so both handlers share one definition, and lowers it from 60 to 15 seconds. startConnectionHandlers() starts handlers sequentially, so the old value let four enabled handlers spend 240 seconds in start(), past the 200 second start-ds timeout; 15 seconds keeps the worst case well inside it and is still generous for a bind(). Logs handlerName instead of friendlyName plus currentConfig.dn() on timeout: handlerName already carries the listen address and port, and nothing is dereferenced on that error path. Reworded the message so its tone matches the error severity it is logged at. TestCaseUtils.assertPortIsAcceptingConnections() replaces the bare connect in the LDAP test and is shared with the new HTTP test. It separates the two failure modes a plain ConnectException conflates -- the handler never bound, versus start() returned too early -- and both tests now assert the handler thread actually stopped after join(). Documents in both start() overrides why they are deliberately not synchronized, matching the dismissal rationale of the java/non-sync-override alerts on the sibling handlers. --- .../reactive/LDAPConnectionHandler2.java | 15 +-- .../opends/server/api/ConnectionHandler.java | 16 +++ .../protocols/http/HTTPConnectionHandler.java | 18 ++-- .../org/opends/messages/protocol.properties | 8 +- .../java/org/opends/server/TestCaseUtils.java | 57 +++++++++++ .../http/HTTPConnectionHandlerTestCase.java | 97 +++++++++++++++++++ .../ldap/TestLDAPConnectionHandler.java | 10 +- 7 files changed, 194 insertions(+), 27 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/protocols/http/HTTPConnectionHandlerTestCase.java 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 f1f9afcdaa..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 @@ -133,12 +133,6 @@ public void run() { /** SSL instance name used in context creation. */ private static final String SSL_CONTEXT_INSTANCE_NAME = "TLS"; - /** - * Maximum time the start method waits for the 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. - */ - private static final long LISTEN_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(60); - private LDAPListener listener; /** The current configuration state. */ @@ -692,6 +686,13 @@ 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 @@ -704,7 +705,7 @@ public void start() { while (!listenAttempted) { final long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadlineNanos - System.nanoTime()); if (remainingMs <= 0) { - logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, friendlyName, currentConfig.dn(), + logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, handlerName, TimeUnit.MILLISECONDS.toSeconds(LISTEN_TIMEOUT_MS)); break; } 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 4521c4a398..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 @@ -120,14 +120,6 @@ 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() { @@ -602,7 +602,7 @@ public void start() final long remainingMs = TimeUnit.NANOSECONDS.toMillis(deadlineNanos - System.nanoTime()); if (remainingMs <= 0) { - logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, friendlyName, currentConfig.dn(), + logger.error(ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER, handlerName, TimeUnit.MILLISECONDS.toSeconds(LISTEN_TIMEOUT_MS)); break; } 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 45dc9c330f..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,10 +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 defined in \ - configuration entry %s did not attempt to open its listen port within %d \ - seconds. The Directory Server startup will continue, but that port might \ - not be accepting connections yet +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 03630f12d5..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 @@ -20,8 +20,6 @@ import static org.opends.server.config.ConfigConstants.*; import static org.testng.Assert.*; -import java.net.InetSocketAddress; -import java.net.Socket; import java.util.Collection; import java.util.LinkedList; import java.util.List; @@ -162,22 +160,20 @@ public void testStartWaitsForListenPort() throws Exception { "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. - int listenPort = handler.getListeners().iterator().next().getPort(); - try (Socket socket = new Socket()) - { - socket.connect(new InetSocketAddress("127.0.0.1", listenPort), 1000); - } + TestCaseUtils.assertPortIsAcceptingConnections(listenPort); } finally { handler.processServerShutdown(reasonMsg); handler.finalizeConnectionHandler(reasonMsg); handler.join(10000); + assertFalse(handler.isAlive(), "the connection handler thread is still running"); } }