[#1119] Serve each LDAP connection handler with a transport of its own, built from its configuration

[vharseko]

Fixes #1119

Summary

LDAPConnectionHandler2, the class every shipped LDAP, LDAPS and administration listener runs on, bound its listeners to the transport the SDK shares across the JVM. So use-tcp-keep-alive, use-tcp-no-delay, buffer-size and num-request-handlers had no effect, and allow-tcp-reuse-address only reached the "address in use" pre-check. Each handler now gets a transport of its own, built from its configuration, as HTTPConnectionHandler already does.

Changes

SDK: GrizzlyLDAPListener

• New option GRIZZLY_TRANSPORT, modelled on GrizzlyLDAPConnectionFactory.GRIZZLY_TRANSPORT. The listener binds to that transport and serves its connections with it. It does not shut the transport down when it is closed: the owner stays responsible for it. Without the option, nothing changes.

LDAPConnectionHandler2

• Each listener gets its own TCPNIOTransport:
  • num-request-handlers sets the number of selector threads. When it is unset, the system property org.forgerock.opendj.transport.selectors is used, so JVMs tuned with it keep their setting. Without the property, the count is max(2, cpus/2), as for the legacy and HTTP handlers.
  • allow-tcp-reuse-address sets SO_REUSEADDR on the listen socket.
  • buffer-size sets the write buffer only, matching its description ("LDAP response message write buffer") and ADMIN_WRITE_BUFFER_SIZE. It is deliberately not the read buffer: every shipped handler has ds-cfg-buffer-size: 4096 bytes explicitly, and 4 KiB reads would be a regression. Today reads are bounded by SO_RCVBUF.
• use-tcp-keep-alive and use-tcp-no-delay are passed as LDAPListener.SO_KEEPALIVE and TCP_NO_DELAY. LDAPServerFilter applies the listener options to every accepted socket after the transport has applied its own, so the transport cannot carry them.
• All handler transports share one PooledMemoryManager. It pre-allocates 3% of the heap as direct buffers, so one per transport would multiply that by the number of handlers. Memory use stays what the shared transport had.
• Stopping the listener (disable, delete, or restart after a component restart) no longer closes the connections it accepted, as with the shared transport. Its transport keeps serving them and is shut down, from a thread of its own, after the last of them closes. Each transport tracks its own connections, so a handler that stops and starts listening again in the same instance (an SSL change it cannot use, then undone) does not keep the old transport alive for the new one's clients. When the server shuts down, those connections are ended as SERVER_SHUTDOWN with a notice of disconnection, then the transport is shut down. The server does not disconnect clients itself on shutdown: that used to happen when the last listener released the shared transport.
• A handler remembers the server instance it was initialized for, and a stopped transport checks whether that instance is shutting down, not the current one. The handler thread stops the listener up to a second after the handler is finalized, and after an in-core restart the current instance is by then the next one. If that server is already shutting down, the connections are ended with the shutdown notice straight away, without registering a shutdown listener: the server may already have notified its listeners.
• Before a transport is shut down, it waits up to 2 seconds for the connections it accepted to close. A notice of disconnection queued behind a response the client has not read yet is written once the client reads, and shutdownNow() would drop it. A client that does not read within those 2 seconds still gets EOF.

Configuration: LDAPConnectionHandlerConfiguration.xml

• For the LDAP connection handler, use-tcp-keep-alive and use-tcp-no-delay are now defined locally instead of referenced from Package.xml, and require component-restart; buffer-size requires it too. The transport fixes these values when the listener starts, like accept-backlog, max-request-size, num-request-handlers and allow-tcp-reuse-address, which already required it. Package.xml is not touched: the HTTP connection handler and the LDAP pass-through policy share those definitions and apply them.

opendj-server-legacy now declares its dependency on opendj-grizzly; it used to come in only through opendj-server.

Behaviour changes worth noting

• Threads: instead of one shared selector pool of max(5, cpus/2 - 1) threads, each handler has its own. With the defaults: max(2, cpus/2) for LDAP and for LDAPS, and 4 for the administration connector.
• dsconfig: changing use-tcp-keep-alive, use-tcp-no-delay or buffer-size on an LDAP connection handler now reports that a restart of the handler is required. The XML is shared by both classes, and ADM cannot set the requirement per class. So the same is reported for a handler that names the legacy org.opends.server.protocols.ldap.LDAPConnectionHandler explicitly, although that class applies these values to the next accepted connection. Since dsconfig create-connection-handler --type ldap creates the legacy LDAPConnectionHandler, not the LDAPConnectionHandler2 the server ships with #1116 new handlers default to LDAPConnectionHandler2.
• Shutdown: stopping a transport can take up to 2 seconds longer, while a client that has not read its responses takes its notice of disconnection.
• Performance: with the shipped buffer-size of 4096, a response larger than about 6 KiB is now written in several write() calls. Grizzly caps a write at 3/2 of the write buffer. Before, the cap was the size of the socket send buffer. No benchmark has been run.

Tests

• New LDAPConnectionHandler2TransportTestCase (19 cases) runs a real handler built from a configuration entry and inspects the server-side sockets:

  • keep-alive and no-delay, all four combinations, on the accepted socket;
  • the connection is served by the handler's own transport, with the configured write buffer;
  • SO_REUSEADDR on the listen socket, true and false. The cases whose handler has no SO_REUSEADDR take a port that a bind without it accepts on 127.0.0.1: TestCaseUtils.findFreePort() checks with SO_REUSEADDR only, and every test class counts ports down from 65535, so a port can still carry a connection an earlier class left in TIME_WAIT;
  • selector threads from num-request-handlers, from the system property, from the processor count, and num-request-handlers taking precedence over the property. Each case also checks that stopping the handler, with no connection open, stops its threads;
  • a listener that cannot bind leaves no selector threads;
  • a stopped handler keeps its connection open, and its threads stop once the connection is closed;
  • a server shutdown ends that connection with a notice of disconnection, and the threads stop. The notice is decoded: OID, result code 52 (UNAVAILABLE) and the shutdown reason. The drain is registered as a shutdown listener;
  • a notice queued behind data the client has not read yet still reaches it, and the transport shuts down within a second of the connection closing;
  • an in-core restart of the test server ends a connection of its LDAP handler with the notice. The notice is read while the server restarts;
  • a handler whose server is shutting down, while another server instance is current, ends its connections with the notice and registers no shutdown listener;
  • with two transports in one handler, the drained one ends with its own connections, and its server-shutdown notice reaches only them.

• GrizzlyLDAPListenerTestCase#testLDAPListenerWithProvidedTransport: the listener serves its connections on the given transport and leaves it running when closed.

• Reactor run (-pl opendj-server-legacy -am, precommit): RejectedSSLConfigurationChangeTestCase 24/24. LDAPConnectionHandler2TransportTestCase run directly with TestNG: 19/19, three runs in a row. In round 3, with ports 65505–65533 held the way CI left them (a bind with SO_REUSEADDR passes, one without it fails), the round-2 test class fails the same two cases as the ubuntu/JDK 11 leg, on the same ports, and the round-3 one passes 19/19; it also passes 19/19 twice on free ports. GrizzlyLDAPListenerTestCase 11/11 in the first round; the SDK has not changed since.

• Mutation check, first round: 14 of 14 mutants fail the new test class. Each removes one piece:

  • one of the listener options: keep-alive, no-delay, GRIZZLY_TRANSPORT;
  • the SDK honouring GRIZZLY_TRANSPORT;
  • the transport settings: write buffer, reuse address, selector threads, selector runners, the system property fallback;
  • the lifecycle: the notice on server shutdown, the transport shutdown, keeping connections open, shutting down after the last connection, the connection-closed hook.

• Mutation check, review round 1: 11 of 13 mutants fail the class:

  • a drain testing or disconnecting the handler's connections instead of its transport's;
  • no server-shutdown arm, no registration, no idle shutdown, a failed listen skipping the drain;
  • the property winning over num-request-handlers;
  • no wait before the shutdown, no connection probe;
  • another disconnect reason, a null message.

  The two survivors were the late isShuttingDown() read and deregistering a drain that was never registered.

• Mutation check, review round 2: 13 of 16 mutants fail the class. The late isShuttingDown() read is now pinned: checking the current instance instead of the handler's fails the new case, and so does dropping the check after registration. The three survivors each differ only inside a window a test cannot hit on demand:

  • deregistering a drain that was never registered;
  • registering without first checking the server, which matters only between getNewInstance and bootstrapServer;
  • not deregistering a drain that finished while it registered, a window between two statements.

Related: #1116 / #1124 make dsconfig create-connection-handler --type ldap create handlers on LDAPConnectionHandler2, so this fix also covers the handlers administrators create.

[maximthomas]

praise: The handler now applies its own transport settings, and they are checked on real sockets.

• LDAPConnectionHandler2.newTransport() keeps what the SDK's shared server transport had (SameThreadIOStrategy, a pooled memory manager, one selector runner per selector thread). Only the configured values change, and MemoryManagerHolder keeps the 3% heap pre-allocation from being multiplied per handler.
• LDAPConnectionHandler2TransportTestCase reads SO_KEEPALIVE, TCP_NODELAY, SO_REUSEADDR and the write buffer from the server-side sockets of a real handler. It passes 12/12 at d7ddc56 in a local failsafe run, with opendj-grizzly and the config XML built from the head.

---

issue (non-blocking): A TransportDrain waits until the whole handler has no connections, not until its own transport has none.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:166-171, :768-783, :830

connectionClosed() tests clientConnections.isEmpty(), and that set holds the connections of every transport of the instance, the live one included. One instance can run two transports. Take an LDAPS handler with clients and set ssl-cert-nickname to an alias the key store does not have. The change is accepted, because createSSLContext(config, false) only logs (:1164-1169). The apply then disables the instance through disableAndWarnIfUseSSL (:1122). ds-cfg-enabled stays true, so ConnectionHandlerConfigManager keeps the instance. run() drains T1, and after the alias is set back it starts T2 in the same instance. From then on, T1 and its numRequestHandlers selector threads stay alive until the handler has no connections at all, which on a busy handler never happens. Each such cycle adds one more transport. Clients are not affected. The class javadoc and the description both say the transport is shut down "after the last one closes". Traced, not run.

// startListener(): record which connections this transport accepts
transport = newTransport();
final Collection<ClientConnection> accepted = ConcurrentHashMap.newKeySet();
transportConnections = accepted;
// ... in apply(): clientConnections.add(conn); accepted.add(conn);
// ... in the three event methods: connectionClosed(conn, accepted);

private void connectionClosed(ClientConnection connection, Collection<ClientConnection> accepted) {
    accepted.remove(connection);
    clientConnections.remove(connection);
    for (TransportDrain drain : drains) {
        drain.connectionClosed();
    }
}

// stopListener(): new TransportDrain(transport, transportConnections)
// TransportDrain keeps that collection as `accepted`, tests `accepted.isEmpty()` in connectionClosed(),
// and disconnects `accepted` in processServerShutdown().

---

issue (non-blocking): Whether a handler's connections get the server-shutdown notice depends on a late isShuttingDown() read, and an in-core restart can race it.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:777, :439-441

stopListener() runs on the handler thread up to 1 s after finalizeConnectionHandler, because of StaticUtils.sleep(1000) in run(). DirectoryServer.shutDown never joins that thread. restart() then calls getNewInstance (DirectoryServer.java:1087), which creates a new instance with shuttingDown=false. If the late read sees that instance, the old handler's connections take drain.start() instead: no notice, they stay open across the restart, and the drain registers with the new server. If the read lands between :1087 and bootstrapServer (:1173), shutdownListeners is still null, so registerShutdownListener (:4103) throws an NPE on the handler thread. At BASE, whether connections survived a restart already depended on timing. What is new is that the promised notice can be skipped and that the NPE is possible. Not run: this needs a restart with a Handler2 connection open, repeated until the late read shows up.

/** Whether the server was shutting down when this handler was finalized. */
private volatile boolean serverShuttingDown;

@Override
public void finalizeConnectionHandler(LocalizableMessage finalizeReason) {
    // DirectoryServer.shutDown sets the flag before it finalizes the handlers, on this thread.
    serverShuttingDown = DirectoryServer.getInstance().isShuttingDown();
    closeReason = finalizeReason;
    // ...
}

// stopListener()
if (serverShuttingDown) {
    drain.processServerShutdown(closeReason);
} else {
    drain.start();
}

---

issue (non-blocking): At server shutdown the drain calls shutdownNow() straight after the disconnects, so a notice queued behind a write backlog may be dropped.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:179-189

For an idle client, Grizzly writes the notice on the calling thread, and serverShutdownEndsTheConnectionsOfAStoppedHandler pins that case. Now take a client that has stopped reading a large result, with roughly 0.5 MiB or more unread on loopback. Its notice is queued behind the backlog, and shutdownNow() terminates the connection without waiting for the queue. That client gets EOF or a reset instead of the notice. The connection ends either way. Not traced into AbstractNIOAsyncQueueWriter or NIOTransport.finalizeShutdown at the Grizzly version the build resolves, and not reproduced. If a queued write can be dropped, wait a bounded time for the disconnect writes, or shut the transport down gracefully, before shutdownNow().

---

suggestion (non-blocking): No case checks that stopping a handler with no open connection shuts its transport down.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:268-281

This is the path taken whenever a handler with no clients is disabled, deleted or restarted: TransportDrain.start() ends with connectionClosed(), which shuts the transport down at once. Both cases that call assertThreadsStop stop their handler with a connection still open, so they end the drain some other way. The mutant that deletes that connectionClosed() call leaks the selector threads on every idle stop, and the class stays green.

private static void assertSelectorThreads(LDAPConnectionHandlerCfg config, int expected) throws Exception
{
  final LDAPConnectionHandler2 handler = start(config);
  final String threadPrefix = selectorThreadPrefix(handler);
  try
  {
    final TCPNIOTransport transport = transport(handler);
    assertEquals(transport.getSelectorRunnersCount(), expected, "selector runners");
    assertEquals(transport.getKernelThreadPoolConfig().getMaxPoolSize(), expected, "selector threads");
  }
  finally
  {
    stop(handler);
  }
  assertThreadsStop(threadPrefix);
}

---

suggestion (non-blocking): The server-shutdown test calls the drain directly, so neither the drain's registration nor stopListener's isShuttingDown() arm is pinned.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:226-229

Two mutants survive the whole class. The first deletes DirectoryServer.registerShutdownListener(this) from TransportDrain.start(). The second replaces the isShuttingDown() arm with drain.start(). The test's stop() never makes the server shut down, so stopListener() always takes drain.start(). No other suite asserts a shutdown-time notice on a Handler2 connection.

      final Collection<?> drains = (Collection<?>) field(handler, "drains");
      assertEquals(drains.size(), 1, "no transport is left serving the connection of the stopped handler");
      final Object drain = drains.iterator().next();
      assertTrue(((Collection<?>) field(DirectoryServer.getInstance(), "shutdownListeners")).contains(drain),
          "the drain is not registered as a shutdown listener");

      ((ServerShutdownListener) drain).processServerShutdown(STOP_REASON);

Pin for the arm: a case that holds a raw socket on the test server's own LDAP port (config.ldif runs LDAPConnectionHandler2 there), calls TestCaseUtils.restartServer(), and asserts the notice-of-disconnection OID arrives. The same case measures the restart race above.

---

suggestion (non-blocking): No case covers a failed listen, where the transport started before the bind fails.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:819-821, :944-945

startListener() starts the transport before new LDAPListener(...) binds. When the bind fails, the transport is drained only by the transport != null arm of stopListener(). The mutant that returns from stopListener() when listener == null leaks two transports per consecutive-failure cycle, and nothing fails. Both initializeConnectionHandler and isConfigurationAcceptable check the port first, so the test has to take the port between initialization and start.

@Test
public void failedListenLeavesNoSelectorThreads() throws Exception
{
  final int port = TestCaseUtils.findFreePort();
  final LDAPConnectionHandler2 handler = new LDAPConnectionHandler2();
  handler.initializeConnectionHandler(DirectoryServer.getInstance().getServerContext(),
      configuration(port, true, true, false, 2));
  try (ServerSocket taken = new ServerSocket())
  {
    taken.bind(new InetSocketAddress("127.0.0.1", port));
    handler.start();
    final long deadline = System.currentTimeMillis() + TIMEOUT_MS;
    while ((Boolean) field(handler, "enabled"))
    {
      assertTrue(System.currentTimeMillis() < deadline, "the handler did not give up listening");
      Thread.sleep(50);
    }
    assertThreadsStop(handler.getConnectionHandlerName() + " Request Handler");
  }
  finally
  {
    stop(handler);
  }
}

---

suggestion (non-blocking): No case pins that num-request-handlers takes precedence over org.forgerock.opendj.transport.selectors.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:143-147

getNumRequestHandlers(config) is correct, but no case sets both the property and num-request-handlers. A mutant that lets the property win whenever it is set stays green.

@Test
public void numRequestHandlersTakesPrecedenceOverTheSelectorsProperty() throws Exception
{
  final String saved = System.getProperty(SELECTORS_PROPERTY);
  System.setProperty(SELECTORS_PROPERTY, "13");
  try
  {
    assertSelectorThreads(configuration(TestCaseUtils.findFreePort(), true, true, true, 3), 3);
  }
  finally
  {
    restore(saved);
  }
}

---

suggestion (non-blocking): The notice assertion accepts any notice of disconnection, whatever its result code and message.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:239-240

LDAPClientConnection2.disconnect sends that OID for every DisconnectReason. So a drain that passes another reason, or a null message (which sends the generic text), stays green.

      final String text = new String(received.toByteArray(), StandardCharsets.ISO_8859_1);
      assertTrue(text.contains(NOTICE_OF_DISCONNECTION_OID), "the connection was closed without a notice of disconnection");
      assertTrue(text.contains(STOP_REASON.toString()), "the notice does not carry the shutdown reason");

Or: to also pin the result code (UNAVAILABLE, 52), decode the response with the SDK's LDAP reader.

---

suggestion (non-blocking): On 4-vCPU CI runners the processor-based selector count is 2, so the test cannot tell it from a constant 2.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:165-175

Math.max(2, cpus / 2) is 2 on any host with 5 or fewer CPUs, so a new-delegation mutant that passes 2 instead of null stays green on ubuntu-latest. The arm itself is BASE code. At most, add a comment saying the case is decisive only on hosts with 6 or more CPUs.

---

note (non-blocking): Change not described in the PR:

• opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/LDAPConnectionHandlerConfiguration.xml:91-108, :124-141, :396-406 — the XML is shared, so the new component-restart on use-tcp-keep-alive, use-tcp-no-delay and buffer-size also applies to handlers still on the legacy org.opends.server.protocols.ldap.LDAPConnectionHandler, the XML's default java-class. That class reads these values for each accepted connection, so dsconfig now reports a restart it does not need.

[vharseko]

All ten points taken in 70ec861, on top of the first commit rebased onto master d0ce1d7. #1116 has been merged, and the rebase dropped only the XML header year change, which #1116 already made.

1. A drain waits for the whole handler. Fixed as suggested. startListener() gives each transport its own set of accepted connections, the event listeners remove a connection from the set of the transport that accepted it, and TransportDrain tests and disconnects only that set. New cases run one instance on two transports, the way the SSL alias change does (enabled set to false and back):

• drainEndsWithTheConnectionsOfItsOwnTransport: closing the connection of the drained transport stops that transport while the live one keeps a connection;
• drainEndsAtServerShutdownOnlyTheConnectionsOfItsOwnTransport: the drain's shutdown sends the notice on its own connection and leaves the other one open.

2. Late isShuttingDown() read. Fixed as suggested: finalizeConnectionHandler records the flag, and stopListener() reads the field. One more thing on the same path: with only that change, the direct arm still goes through finish(), which called DirectoryServer.deregisterShutdownListener. Between getNewInstance and bootstrapServer that is the NPE from your trace, just on the deregistering side. It would escape run() and leave the transport running. A drain now deregisters only if start() registered it, and the direct arm never touches the registry. What remains of the race is the bookkeeping of the late disconnect itself (DirectoryServer.connectionClosed, access log, post-disconnect plugins), which lands on whichever instance is current. The legacy handler closes its clients on its own thread after the same one-second sleep, so this PR leaves that part as it found it.

3. shutdownNow() right after the disconnects. Confirmed at Grizzly 3.0.1: finalizeShutdown → stopSelectorRunners → SelectorRunner.shutdownSelector → terminateSilently, which fails every record still in the async write queue. Waiting for the handler's own set to empty does not help, because handleConnectionDisconnected fires before the notice is written (LDAPServerFilter.disconnect). So each transport now has a ConnectionProbe that tracks the Grizzly connections it accepted and has not closed yet. LDAPServerFilter closes a connection only after the notice is written, and before shutdownNow() the drain waits up to 2 s (NOTICE_DELIVERY_TIMEOUT_MS) for all of them to close. The wait covers the server-shutdown arm and the last-connection arm alike. On the last-connection arm it matters when the last connection is ended by the server with a notice, for instance by an idle time limit. A client that does not read within those 2 s still gets EOF: the bound keeps a stuck client from holding up the shutdown. New case serverShutdownDeliversANoticeQueuedBehindUnreadData fills the socket buffers and then the write queue with raw bytes below the LDAP filters, checking that the queue is not empty. It shuts the drain down on another thread and only then starts reading: all the bytes, then the notice, then EOF.

4. Idle stop. assertSelectorThreads now ends with assertThreadsStop, as in your snippet. That covers the idle stop in the four selector cases.

5. Registration and the shutdown arm. serverShutdownEndsTheConnectionsOfAStoppedHandler asserts the drain is in shutdownListeners before calling it. New serverRestartEndsTheConnectionsOfAListeningHandler holds a raw socket on the test server's LDAP port, calls TestCaseUtils.restartServer(), and expects the notice with INFO_CONNHANDLER_CLOSED_BY_SHUTDOWN. With the flag read at finalization it is deterministic.

6. Failed listen. failedListenLeavesNoSelectorThreads, your snippet as is.

7. Precedence. numRequestHandlersTakesPrecedenceOverTheSelectorsProperty, your snippet as is.

8. Notice content. assertNoticeOfDisconnection decodes the message with the server's LDAPReader. It checks the OID, result code 52 (UNAVAILABLE) and the diagnostic message, then EOF. All four notice cases use it.

9. Processor count. Comment added: the case is decisive only on hosts with 6 or more CPUs.

10. component-restart on the legacy class. Since #1116 the XML's default java-class is LDAPConnectionHandler2, so this concerns only handlers that name the legacy class explicitly. ADM cannot set the requirement per class, so the description now says so under "Behaviour changes".

Checks. Directly with TestNG: LDAPConnectionHandler2TransportTestCase 18/18, three runs in a row. RejectedSSLConfigurationChangeTestCase 24/24 in a reactor run (-pl opendj-server-legacy -am). Mutation check: 13 mutants for this round, 11 fail the class:

• the drain testing or disconnecting the handler's connections instead of its own;
• no server-shutdown arm;
• no registration;
• no idle shutdown;
• a failed listen skipping the drain;
• the property winning over num-request-handlers;
• no wait before shutdownNow();
• no probe;
• another DisconnectReason;
• a null message.

Two survive, both parts of the restart race that a test cannot hit on demand: going back to the late isShuttingDown() read, and deregistering an unregistered drain. Both only differ in the window between getNewInstance and bootstrapServer.

[maximthomas]

praise: Round 2 fixes all three round-1 findings where they occur, and each fix has a case that exercises it.

• Each TransportDrain now waits on and disconnects only its own transport's accepted set, so a stop/start in one instance no longer keeps the old transport alive (drainEndsWithTheConnectionsOfItsOwnTransport).
• serverShuttingDown is now recorded in finalizeConnectionHandler instead of being read late on the handler thread, and the OpenConnections probe lets a notice queued behind unread data go out before shutdownNow(). serverShutdownDeliversANoticeQueuedBehindUnreadData fills the Grizzly write queue to prove it, and passed locally in 1.3 s.
• 17 of the 18 cases in LDAPConnectionHandler2TransportTestCase passed locally at 70ec861.

---

issue (non-blocking): serverRestartEndsTheConnectionsOfAListeningHandler fails on macOS whenever the in-core restart takes longer than about 60 s.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:327-337, :520

The case reads the notice only after TestCaseUtils.restartServer() returns, so the notice and the server's FIN wait in the client's receive buffer for the whole restart. In a local macOS run the server sent the notice 1 s into the shutdown (DISCONNECT conn=10 reason="Server Shutdown"). The restart then took 95 s, most of it a reverse-DNS stall at start-up on that machine, and the read failed with java.net.SocketException: Connection reset (tests=18, failures=1). macOS resets a loopback connection whose server side has closed once net.inet.tcp.fin_timeout (60 s) expires, and the client's unread data is lost. A standalone probe confirms it: a read after 5 s and after 50 s returned the data, a read after 75 s got Connection reset. Linux cells are expected to be green, but this is not confirmed yet. Reading while the restart runs removes the dependency on how long it takes:

    try (Socket client = new Socket("127.0.0.1", TestCaseUtils.getServerLdapPort()))
    {
      awaitServerConnection(client.getLocalPort());
      // Read while the server restarts: macOS resets a closed loopback connection after
      // net.inet.tcp.fin_timeout (60 s) and drops what the client has not read yet.
      final ExecutorService reader = Executors.newSingleThreadExecutor();
      try
      {
        final Future<?> notice = reader.submit(() -> {
          assertNoticeOfDisconnection(client, INFO_CONNHANDLER_CLOSED_BY_SHUTDOWN.get());
          return null;
        });
        TestCaseUtils.restartServer();
        notice.get(TIMEOUT_MS, TimeUnit.MILLISECONDS);
      }
      finally
      {
        reader.shutdownNow();
      }
    }

---

issue (non-blocking): If a handler is disabled or deleted less than a second before an in-core restart, its drain registers after shutDown has already notified the shutdown listeners.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:203-208, :512, :852

A disable or delete finalizes the handler with serverShuttingDown=false and deregisters it, so shutDown never finalizes it again. Its thread leaves sleep(1000) up to a second later and takes drain.start(). If shutDown's listener loop (DirectoryServer.java:4193-4195) has already run by then, the drain is never notified. Its clients get no notice, and the drained transport keeps serving them across the restart, which is also what happened at BASE. In an embedded server (daemon threads, so the swap does not wait for the handler thread), registerShutdownListener can also hit the null shutdownListeners between getNewInstance and bootstrap. The NPE then ends the thread before connectionClosed() runs. Re-checking after registering closes the first road:

        private void start() {
            drains.add(this);
            registered = true;
            DirectoryServer.registerShutdownListener(this);
            if (DirectoryServer.getInstance().isShuttingDown()) {
                // Registered after shutDown notified its listeners: end the connections as it would have.
                processServerShutdown(closeReason);
            } else {
                connectionClosed();
            }
        }

---

issue (non-blocking): TransportDrain.start() adds the drain to drains before it registers it, so if the last connection closes in between, a finished drain stays registered as a shutdown listener.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:203-208, :236-246

Suppose a selector thread closes the drained transport's last connection between drains.add(this) and registerShutdownListener(this). finish() then wins the CAS and either skips deregistration or removes a listener that is not registered yet. The handler thread then registers the finished drain, which stays in DirectoryServer.shutdownListeners with the handler and the shut-down transport until the next shutdown. With the fix below, whichever of the two runs last removes the listener:

            DirectoryServer.registerShutdownListener(this);
            if (finished.get()) {
                // The last connection closed between drains.add and the registration, and finished the drain first.
                DirectoryServer.deregisterShutdownListener(this);
            }

---

suggestion (non-blocking): No test covers the close half of OpenConnections. A drain that always waits the full 2 s still passes every case.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:162-169, test :313-314

If onCloseEvent does nothing, or awaitClosed becomes a flat 2 s wait, every drain and every server shutdown takes 2 s longer, and every case still passes. The queued-notice case joins the drain thread for TIMEOUT_MS (10 s), and no case bounds elapsed time below that.

      assertNoticeOfDisconnection(client, STOP_REASON);
      // The client has read everything and the server has closed: the drain must not wait out its 2 s bound.
      shutdown.join(1000);
      assertFalse(shutdown.isAlive(), "the drain is still waiting for a closed connection");

Pin: with a no-op onCloseEvent the drain thread is still in awaitClosed at about 1.3 s, so the case fails. At HEAD the thread ends about 0.3 s after the notice is read.

---

suggestion (non-blocking): serverRestartEndsTheConnectionsOfAListeningHandler catches a revert of the finalize-time serverShuttingDown only when the handler thread wakes after getNewInstance.

opendj-server-legacy/src/test/java/org/opends/server/protocols/ldap/LDAPConnectionHandler2TransportTestCase.java:322-337

The embedded test server forces daemon threads, so getNewInstance does not wait for the handler thread. A revert to DirectoryServer.getInstance().isShuttingDown() in stopListener therefore takes the same road as HEAD whenever the thread wakes while the old instance is still shutting down. In the local run the notice went out at 16:09:15 and the old instance stopped at 16:09:18, so the thread read the flag about 3 s before the swap, and the revert would have passed. To pin the fix, the test needs a way to hold the handler thread until the swap has happened. Otherwise, the javadoc should drop "after the server has moved on", because the case does not force that order.

---

suggestion (non-blocking): No case reaches the catch arm in newTransport, which shuts down a transport whose start() failed.

opendj-server-legacy/src/main/java/org/forgerock/opendj/reactive/LDAPConnectionHandler2.java:888-893

In failedListenLeavesNoSelectorThreads the listen fails at the GrizzlyLDAPListener bind, after start() has succeeded, so the drain cleans up and the catch never runs.

Pin: there is no cheap one. It would need a way to make TCPNIOTransport.start() throw, and the arm is three lines and correct by reading, so it is fine to leave.

[vharseko]

Thanks for the second round. Points 1–5 are taken in 5aa486c, and point 6 is left as you suggested. The branch is rebased onto master 3295ece (#1123) without conflicts.

1. The restart case on macOS. Taken as you wrote it: the notice is read on another thread while TestCaseUtils.restartServer() runs, and the comment names net.inet.tcp.fin_timeout.

2. A drain started after shutDown notified its listeners. Fixed, a little differently from the snippet. After the swap, DirectoryServer.getInstance().isShuttingDown() reads the next instance, which is not shutting down. The re-check would then miss the second road, and registering would still meet the null shutdownListeners before bootstrap. So initializeConnectionHandler records the instance it runs for (server), and TransportDrain.start() asks that instance. Its flag never goes back to false. If it is shutting down, the drain ends its connections with INFO_CONNHANDLER_CLOSED_BY_SHUTDOWN and does not register. Otherwise it registers and checks again, as in your snippet. With the handler's own instance, the flag recorded in finalizeConnectionHandler in round 1 is no longer needed, and serverShuttingDown and closeReason are gone. The handler-thread path of an in-core restart now takes the same road: its server is shutting down, so the drain ends the connections directly. One narrow window remains: the whole shutdown and getNewInstance running between the check and registerShutdownListener, two consecutive statements.

3. A finished drain left registered. Taken as suggested: after registering, a drain that is already finished deregisters itself.

4. The close half of OpenConnections. Taken. The queued-notice case now joins the drain thread for CLOSED_DRAIN_ENDS_MS (1 s) after reading the notice, and asserts it has ended. A mutant whose onCloseEvent does not notify now fails that case.

5. The restart case does not force the order. New case drainEndsTheConnectionsWhenTheServerOfItsHandlerIsShuttingDown covers it without a real restart. It gives the handler a stand-in DirectoryServer, built with the private constructor and marked as shutting down, while the test server stays current, then stops the handler the way a disable does. It expects the shutdown notice, no drain left, and no drain among the current server's shutdown listeners. Two mutants fail it: asking the current instance instead of the handler's, and dropping the re-check. The javadoc of the restart case no longer says "after the server has moved on" and points to the new case.

6. The catch arm in newTransport. Left as is, as you suggested.

Checks. LDAPConnectionHandler2TransportTestCase, run directly with TestNG: 19/19, three runs in a row. Mutation check for this round: 16 mutants, 13 fail the class. The three survivors each differ only inside a window a test cannot hit on demand:

• deregistering a drain that was never registered, as in round 1;
• registering without first checking the server, which matters only between getNewInstance and bootstrapServer;
• not deregistering a drain that finished while it registered (point 3), a window between two statements.

The PR description is updated to match.

[vharseko]

CI: the red ubuntu/JDK 11 leg. Fixed in a96d796. It is a test fixture problem, not a problem in the handler.

The leg failed in both CI runs of this PR, on 70ec861 and on 5aa486c. Each time the same two cases of LDAPConnectionHandler2TransportTestCase failed, and they are the only two whose handler has allow-tcp-reuse-address: false: failedListenLeavesNoSelectorThreads and listenSocketUsesTheConfiguredReuseAddress[false]. The 17 cases with SO_REUSEADDR passed. The failure was already in the "address in use" check of initializeConnectionHandler (unable to bind to 127.0.0.1:6552x: Address already in use), before any transport of this PR exists. That check is not changed here.

The cause is in the ports. TestCaseUtils.findFreePort() checks a port with a bind that has SO_REUSEADDR, and failsafe runs every test class in a JVM of its own (reuseForks=false), so each class counts ports down from 65535 again. A port can still carry a connection that an earlier class left in TIME_WAIT. Such a port accepts a bind with SO_REUSEADDR and refuses one without it. No other test in the suite uses allow-tcp-reuse-address: false, so nothing had hit this before. I could not show from the logs why only the JDK 11 leg meets it: the classes run in the same order and with the same timing on every leg.

The two cases now take their port from a small helper, freePort(false). It skips ports that a bind without SO_REUSEADDR refuses on 127.0.0.1. TestCaseUtils is left as it is, since the whole suite shares it.

Checks. The class was run directly with TestNG while another process held ports 65505–65533 in the state CI met: each port had an open connection and no listener, so a bind with SO_REUSEADDR passed and one without it failed. The round-2 class failed the same two cases as CI, on the same ports 65523 and 65521. The round-3 class passed 19/19. It also passed 19/19 twice on free ports. The PR description is updated.

[maximthomas]

praise: The drain now decides on the server it belongs to, and the tightened tests measurably pin the drain.

• TransportDrain.start() tests server, the instance captured in initializeConnectionHandler (LDAPConnectionHandler2.java:681, :206, :215). A drain stopped during an in-core restart therefore never registers with the next instance and never waits for a listener loop that has already run.
• serverShutdownDeliversANoticeQueuedBehindUnreadData now joins for CLOSED_DRAIN_ENDS_MS. Emptying OpenConnections.onCloseEvent turns it red ("the drain is still waiting for a closed connection", 1 of 19 cases red), so the close half of the drain is pinned.
• serverRestartEndsTheConnectionsOfAListeningHandler reads the notice while the server restarts. The class is green 19/19 locally on macOS and on every Linux CI cell.