[#794] Wait for the listen port to be open before returning from connection handler start

[vharseko]

Fixes #794

Problem

org.forgerock.opendj.reactive.LDAPConnectionHandler2 declares a waitListen monitor whose documented purpose is to make server startup block until the listener is up, but nothing ever waits on it. ConnectionHandler.start() is therefore a plain Thread.start(), and DirectoryServer.startServer() can return while the port is still closed — a client connecting immediately afterwards gets Connection refused.

This is the handler that is actually used: config.ldif configures LDAP and LDAPS with ds-cfg-java-class: org.forgerock.opendj.reactive.LDAPConnectionHandler2, and AdministrationConnector wraps the same class, so the administration port (4444) is affected as well — dsconfig, status and dsreplication right after a start can hit a closed port. start-ds inherits the race too: NOTE_DIRECTORY_SERVER_STARTED is logged immediately after startConnectionHandlers().

HTTPConnectionHandler has the same bug by a different route, and fixing it is the second fix in this PR, not a consistency cleanup. It does wait, but it notifies before startHttpServer(), so start() returns while the embedded server is still coming up (and its wait() has no condition predicate, so a spurious wakeup returns from start() early). This is not only about server startup: enabling the handler through dsconfig reports success while the port is still closed.

set-connection-handler-prop --handler-name "HTTP Connection Handler" \
    --set listen-port:<free port> --set enabled:true

Connecting immediately afterwards, with no retry loop, is refused on master and succeeds with this PR. That path is ConnectionHandlerConfigManager.applyConfigurationChange() → connectionHandler.start() (opendj-server-legacy/src/main/java/org/opends/server/core/ConnectionHandlerConfigManager.java:94,135).

Why the legacy pattern cannot be copied verbatim

org.opends.server.protocols.ldap.LDAPConnectionHandler (unused by the default configuration) implements the waiting side and notifies after registerChannels(). LDAPConnectionHandler2 notifies before startListener(), and GrizzlyLDAPListener binds every address synchronously in its constructor — so "the port is open" is equivalent to "startListener() returned". Setting the predicate at the existing notify site would only narrow the race window, not close it.

Change

• listenAttempted predicate guarded by waitListen, set after the bind attempt in a finally block, so a failed bind unblocks startup exactly like the legacy handler's "attempted" semantics.
• start() overrides wait on while (!listenAttempted), guarding against spurious wakeups, and restore the interrupt flag.
• The wait is bounded and logs ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER on expiry: the bind now happens inside the waited region, and a handler thread dying before it reaches the listen code must not hang the whole server startup.
• The bound is a single ConnectionHandler.LISTEN_TIMEOUT_MS shared by both handlers, set to 15 seconds. startConnectionHandlers() starts handlers sequentially and each one spends up to that long in start(), so the worst case is 15 s × number of handlers — well inside the 200 s start-ds timeout (DirectoryServer.DEFAULT_TIMEOUT) even with LDAP, LDAPS, administration and HTTP all enabled, and still generous for a bind().
• Same treatment for HTTPConnectionHandler, so LDAP, LDAPS, administration and HTTP ports are all ordered consistently — see above for why that one is also a fix.
• Both start() overrides document why they are deliberately left unsynchronized, matching the won't fix rationale of the existing java/non-sync-override dismissals on the sibling handlers.

Test

Each handler gets a testStartWaitsForListenPort that connects to the listen port immediately after start() returns, with no retry loop:

test	result	with the handler reverted to master
TestLDAPConnectionHandler	Tests run: 4, Failures: 0	AssertionError: the connection handler start method returned before port N was open
HTTPConnectionHandlerTestCase (new)	Tests run: 1, Failures: 0	AssertionError: the connection handler start method returned before port N was open

Both failures are deterministic — rerunFailingTestsCount=3 does not rescue them. HTTPConnectionHandler.start() had no test coverage at all before this PR; HTTPConnectionHandlerTestCase is the first test to start that handler.

TestCaseUtils.assertPortIsAcceptingConnections() is shared by both tests. It separates the two failure modes a bare ConnectException conflates: the handler never managed to bind, versus start() returned before the port was open. Both tests also assert that the handler thread actually stopped after join(), so a stuck thread cannot leak silently into later tests in the same JVM.

DsconfigOptionsTestCase also still passes (Tests run: 7, Failures: 0).

Notes

• Rebased onto Fix CodeQL warning-severity alerts: missed wakeups, resource leaks, escaping threads #790 (0885d16), which touched the same lines. This PR supersedes Fix CodeQL warning-severity alerts: missed wakeups, resource leaks, escaping threads #790's HTTPConnectionHandler hunk — same field, same guard, same interrupt restore, plus the bounded wait and the moved notify — and closes 4 of its java/notify-instead-of-notify-all alerts, on the lines rewritten here.
• The new LDAPConnectionHandler2.start() raises a java/non-sync-override alert. It needs the same won't fix dismissal as the two existing ones on LDAPConnectionHandler.java and HTTPConnectionHandler.java: synchronized would add no safety here (the state is guarded by waitListen, and the already-started check belongs to super.start()) while holding the thread's own monitor across waitListen.wait() would block Thread.join().
• startConnectionHandlers() still does not check whether the bind succeeded — the server reports a successful start even when a port could not be opened. That is a separate follow-up, in the spirit of Flaky test: AssuredReplicationServerTest#testSafeDataFromRS — RS silently starts without a listener when the replication port is briefly occupied #792, and this PR builds the mechanism for it: now that start() waits for the bind, reporting success becomes a return value rather than a redesign.
• The legacy org.opends.server.protocols.ldap.LDAPConnectionHandler.start() still waits unbounded. It is unused by the default configuration and untouched here; bounding it with the same shared constant is a small follow-up.

[maximthomas]

The fix is correct and I verified it by building and running, not just reading:

• mvn install passes; ERR_CONNHANDLER_TIMEOUT_WAITING_FOR_LISTENER generates as Arg3<Object, Object, Number>, so (String, DN, long) type-checks.
• TestLDAPConnectionHandler → Tests run: 4, Failures: 0. With LDAPConnectionHandler2 reverted → testStartWaitsForListenPort:173 » Connect Connection refused, deterministic (rerunFailingTestsCount=3 did not rescue it). Genuine regression test.
• GrizzlyLDAPListener binds synchronously in its constructor, so "startListener() returned" really does mean the port is listening — the finally placement is the only one that closes the window.
• listenAttempted is touched only under the waitListen monitor in both classes; wait(remainingMs) can never be reached with 0. Clean.
• Checked and benign: startHttpServer() is now guaranteed to run before httpEndpointConfigManager.registerTo(...) (DirectoryServer.java:1548), where it previously raced. getHTTPRouter() returns the live mutable Router and AbstractRouter.addRoute() calls notifyDescriptorChange(), so late-registered endpoints still route and the API descriptor is rebuilt.

Three things to address before merge.

HTTP handler: an undocumented, untested second bug fix (major)

The description frames the HTTP change as consistency ("Same treatment for HTTPConnectionHandler, so LDAP, LDAPS, administration and HTTP ports are ordered consistently"). It is actually a second bug fix. Enabling the handler through dsconfig currently reports success while the port is still closed:

set-connection-handler-prop --handler-name "HTTP Connection Handler" \
    --set listen-port:<free port> --set enabled:true

then connecting immediately, with no retry loop:

	dsconfig returned after	connect
master	976 ms	ConnectException: Connection refused
this PR	1455 ms	OK

That path is ConnectionHandlerConfigManager.applyConfigurationChange() → connectionHandler.start() (opendj-server-legacy/src/main/java/org/opends/server/core/ConnectionHandlerConfigManager.java:94,135).

The description also says DsconfigOptionsTestCase exercises the HTTP handler through dsconfig. It does not — testSetEnableHTTPConnectionHandler passes no --set at all:

// opendj-server-legacy/src/test/java/org/opends/server/tools/dsconfig/DsconfigOptionsTestCase.java:54
final String[] args =
{
  "set-connection-handler-prop",
  ...
  "--handler-name", "HTTP Connection Handler",   // no --set, nothing is enabled
};

It is the only file in the test tree that mentions the HTTP connection handler, and the handler is ds-cfg-enabled: false in opendj-server-legacy/resource/config/config.ldif:367. HTTPConnectionHandler.start() has no coverage at all.

Please add an HTTP counterpart to testStartWaitsForListenPort and say in the description that the HTTP path is a fix, not a cleanup. Note this also means #790 alone would not fix it — that PR keeps the notify before startHttpServer() and only closes the spurious-wakeup hole.

Per-handler timeout can exceed the start-ds timeout (medium)

startConnectionHandlers() starts handlers sequentially, each with its own 60 s budget:

// opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:3502
for (ConnectionHandler<?> handler : connectionHandlers)
{
  handler.start();
}

start-ds waits via WaitForFileDelete -t, default DirectoryServer.DEFAULT_TIMEOUT = 200 seconds. Default config enables LDAP + the always-started administration connector → 120 s worst case, inside budget. Enable LDAPS and HTTP as well and the bound becomes 240 s, i.e. start-ds reports a failed start for a server that does come up.

Only reachable if handler threads die before the listen code, but the cliff is easy to remove: one deadline shared across the loop, or 10–15 s per handler — ample for a bind().

New CodeQL java/non-sync-override alert (minor)

Thread.start() is synchronized, and neither DirectoryThread nor ConnectionHandler overrides it, so the new LDAPConnectionHandler2.start() is a third override of it. The repo already has two dismissed alerts for this rule, on exactly the sibling methods:

• opendj-server-legacy/src/main/java/org/opends/server/protocols/ldap/LDAPConnectionHandler.java:814
• opendj-server-legacy/src/main/java/org/opends/server/protocols/http/HTTPConnectionHandler.java:571

both won't fix: "Deliberately left unsynchronized. The state touched is guarded by the waitListen monitor … holding the thread's own monitor across waitListen.wait() would block Thread.join()."

The new override needs the same dismissal. The HTTP one may resurface too, since that method body moved (571 → 588).

Nits

• Duplicated constant: LISTEN_TIMEOUT_MS = 60 s is defined verbatim in both handlers and is not configurable.
• currentConfig.dn() in the timeout log: unreachable-null in practice (initializeConnectionHandler always runs first), but it sits on an error path, and an NPE there escapes start() into startConnectionHandlers().
• ERR_ vs WARN_: the message says startup will continue, which reads like a warning, but the key is ERR_ and it is logged at error level.
• Test can't separate its two failure modes: at opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/TestLDAPConnectionHandler.java:167, a failed bind and "start() returned too early" both surface as ConnectException. Asserting the listener is up before connecting would split them; assertFalse(handler.isAlive()) after join(10000) would stop a stuck handler thread leaking silently into later tests in the same JVM.
• Merge with #790: this PR supersedes #790's HTTPConnectionHandler hunk entirely (same field, same guard, same interrupt restore, plus the bounded wait and the moved notify) — resolve by taking this version of the file wholesale. It also closes 4 of #790's 25 java/notify-instead-of-notify-all alerts, which sit on exactly the lines rewritten here (LDAPConnectionHandler2.java:704,722, HTTPConnectionHandler.java:628,649). Landing this first shrinks #790.
• Legacy handler still uses notify(): LDAPConnectionHandler.java:876,900 — briefly inconsistent if this lands before #790.
• Follow-up: deferring the "startConnectionHandlers() never checks whether the bind succeeded" fix is right, and this PR builds the mechanism for it — now that start() waits for the bind, reporting success is a return value rather than a redesign. Worth linking into #792.

[vharseko]

Thanks — all three addressed, plus the nits. Rebased onto master (which now carries #790) and pushed a separate follow-up commit so the delta since your review is reviewable on its own.

HTTP handler: undocumented, untested second bug fix (major)

You were right on both counts, and the claim about DsconfigOptionsTestCase in the old description was simply wrong — testSetEnableHTTPConnectionHandler passes no --set, so it never starts the handler.

Added opendj-server-legacy/src/test/java/org/opends/server/protocols/http/HTTPConnectionHandlerTestCase.java, the first test in the tree that starts HTTPConnectionHandler at all. It builds the handler config through InitializationUtils.getConfiguration(HTTPConnectionHandlerCfgDefn...) against the real ServerContext (a mock is not enough — RootHttpApplication.start() dereferences getHTTPRouter() and getCommonAudit()), starts it, and connects with no retry loop.

Verified the same way you did, by reverting each handler to its master version and re-running:

	with the fix	with the handler reverted to master
HTTPConnectionHandlerTestCase	Tests run: 1, Failures: 0	AssertionError: the connection handler start method returned before port 65531 was open
TestLDAPConnectionHandler	Tests run: 4, Failures: 0	AssertionError: the connection handler start method returned before port 65527 was open

Both deterministic — rerunFailingTestsCount=3 did not rescue either. DsconfigOptionsTestCase still green (Tests run: 7, Failures: 0).

The description now states plainly that the HTTP change is a fix, shows the dsconfig reproducer, and no longer claims coverage that does not exist.

Per-handler timeout vs the start-ds timeout (medium)

Took the "10–15 s per handler" option, and folded the duplicated-constant nit into the same change: the value now lives once in ConnectionHandler.LISTEN_TIMEOUT_MS and is 15 seconds. Worst case with LDAP + LDAPS + administration + HTTP is 60 s against DirectoryServer.DEFAULT_TIMEOUT = 200, so the cliff is gone with a wide margin, and 15 s is still far more than a bind() needs.

I did not go for the shared deadline: threading a deadline into ConnectionHandler.start() means changing the signature of a @PublicAPI(mayExtend=true) method for a safety net that only fires when a handler thread has already died. The javadoc on the constant spells out the sequential-start reasoning so the multiplication is not rediscovered later.

Not configurable — that would mean a new admin-framework property for a value that only bounds a failure path. Happy to add one if you disagree.

java/non-sync-override (minor)

Cannot be fixed in code for the reason your own dismissal gives, so I documented it at the source instead: both start() overrides now carry a javadoc paragraph saying they are deliberately unsynchronized, with the waitListen / Thread.join() rationale. Alert 1235 still needs the same won't fix dismissal as #659 and #660 — that is a Security-tab action, not something the branch can carry. Same for the HTTP one if it resurfaces, since that method moved again (master 578 → 588 here).

Nits

• Duplicated constant — fixed, see above.
• currentConfig.dn() on the error path — removed the dereference entirely rather than null-guarding it. The message now takes handlerName, which is built in initializeConnectionHandler as friendly name + listen addresses + port ("LDAP Connection Handler 0.0.0.0 port 1389"), so it identifies the handler at least as well as the DN and nothing can throw there.
• ERR_ vs WARN_ — kept ERR_ and fixed the tone instead: a handler thread that has not even attempted a bind after 15 s is broken, and that port is very unlikely to work. New text: "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". Generates as Arg2<Object, Number>, matching (String, long). Say the word if you would still rather have it a WARN_.
• Test cannot separate its two failure modes — both tests now go through TestCaseUtils.assertPortIsAcceptingConnections(port). On ConnectException it polls for up to 10 s: if the port opens, the handler can bind and start() returned too early ("...returned before port N was open"); if it never opens, the failure is unrelated ("the connection handler never opened port N"). That is exactly the message both reverted runs above produced. Both tests also assertFalse(handler.isAlive()) after join(10000), so a stuck handler thread cannot leak into later tests in the same JVM.
• Merge with Fix CodeQL warning-severity alerts: missed wakeups, resource leaks, escaping threads #790 — resolved by taking this PR's version of both files wholesale, as you suggested; I diffed the result against the pre-rebase blobs to confirm they are byte-identical.
• Legacy handler still uses notify() — no longer true, Fix CodeQL warning-severity alerts: missed wakeups, resource leaks, escaping threads #790 landed first and converted it. What remains there is that LDAPConnectionHandler.start() still waits unbounded; it is unused by the default configuration and untouched here, but the shared constant now sits in its superclass, so bounding it is a few lines whenever you want it.
• Follow-up — linked into the description: startConnectionHandlers() still ignores whether the bind succeeded, and with start() now waiting for it, reporting that is a return value rather than a redesign.