Notify connection event listeners when an internal connection is closed

[vharseko]

Clears the java/unused-container code scanning alert 692 — "The contents of this container are never accessed".

Problem

InternalConnection maintained a list of connection event listeners that nothing ever read:

private final List<ConnectionEventListener> listeners = new CopyOnWriteArrayList<>();

addConnectionEventListener() added to it and removeConnectionEventListener() removed from it, but no code path ever iterated it. An internal connection therefore never told its listeners anything, silently breaking the documented contract of Connection.addConnectionEventListener():

Registers the provided connection event listener so that it will be notified when this connection is closed by the application, receives an unsolicited notification, or experiences a fatal error.

The same area carried two related defects, each already marked in the source:

public boolean isClosed() {
    // FIXME: this should be true after close has been called.
    return false;
}

public boolean isValid() {
    // FIXME: this should be false if this connection is disconnected.
    return true;
}

And close() forwarded to the server connection on every call, whereas Connection.close() specifies that "calling close on a connection that is already closed has no effect".

Fix

Rather than hand-rolling the notification, this reuses org.forgerock.opendj.ldap.spi.ConnectionState, which already implements the VALID -> CLOSED / ERROR state machine together with the listener bookkeeping — and whose own comment asks for exactly this:

/*
 * FIXME: This class should be used by connection pool and ldap connection
 * implementations as well.
 */

Method	Before	After
addConnectionEventListener	added to a list nobody read	state.addConnectionEventListener(...)
removeConnectionEventListener	removed from that list	state.removeConnectionEventListener(...)
close(UnbindRequest, String)	always called serverConnection.handleConnectionClosed(...)	notifies the listeners and forwards to the server connection only on the first close
isClosed()	return false; + FIXME	state.isClosed()
isValid()	return true; + FIXME	state.isValid()

close() is now idempotent, as the interface requires, and both FIXMEs are resolved.

handleConnectionError and handleUnsolicitedNotification are not wired up. An internal connection has no transport, so it can neither fail nor receive an unsolicited notification; closure by the application is the only event its listeners can observe. This is documented on the new field.

Operation-level state checks (throwing IllegalStateException from addAsync and friends once the connection is closed) are intentionally left out — that is a separate contract change, beyond the scope of this alert.

Review follow-ups

Making InternalConnection notify its listeners exposed two defects in the notification path. Both are fixed here.

A listener may de-register itself while being notified

ConnectionState iterated its LinkedList of listeners while holding its own monitor, and removeConnectionEventListener() is re-entrant on that monitor. A listener de-registering itself from its own callback — the idiom LDAPConnectionFactory uses in handleConnectionClosed() — broke the loop:

Listeners registered	Behaviour
2	only the first is notified, the loop ends silently
3 or more	only the first is notified, then ConcurrentModificationException out of close()

For an internal connection the exception escaped before serverConnection.handleConnectionClosed(...), and since the state was already CLOSED a retried close() was a no-op — the server was never told, and the pending requests were never cancelled.

The listener list is now a CopyOnWriteArrayList, which fixes the close, error and unsolicited notification loops at once, for every connection implementation built on ConnectionState.

A misbehaving listener must not skip the server-side close

Same shape, simpler trigger: a RuntimeException from a listener also escaped close() before the forward. That matters more here than for LDAPConnectionFactory, where a separate connectionImpl.close() still runs — for an internal connection the forward is the only cleanup there is, and RequestHandlerFactoryAdapter.doClose() is what cancels the pending requests. It reaches production code: OpenDJProvider.newLDIFKeyStore builds a newInternalConnectionFactory, and KeyStoreImpl closes those connections in six try-with-resources blocks.

The hand-over is now guarded by an AtomicBoolean and forwarded from a finally block, so it happens exactly once even if the notification fails. The exception still reaches the caller, as it does at every other notification site in the SDK.

Stale javadoc

Connection.addConnectionEventListener() and its ConnectionState counterpart documented an @throws IllegalStateException for a listener registered after the connection was closed. No implementation throws it — ConnectionState and GrizzlyLDAPConnection both notify the late listener immediately — so the javadoc now describes what the code does, which is also what testListenerAddedAfterCloseIsNotifiedImmediately pins down.

Testing

InternalConnectionTestCase — 8 tests covering close notification, close idempotency, listener removal, late registration, the isClosed()/isValid() transition, a self-de-registering listener, a throwing listener, and a 16-thread concurrent close. The close assertions check that the very UnbindRequest the caller passed is the one forwarded.

ConnectionStateTest — 2 tests for a listener de-registering itself during the close and during the error notification.

Run with the original InternalConnection restored, the tests covering the alert itself fail:

InternalConnectionTestCase.testCloseNotifiesListeners                       expected:<1> but was:<0>
InternalConnectionTestCase.testCloseIsIdempotent                            expected:<1> but was:<0>
InternalConnectionTestCase.testListenerAddedAfterCloseIsNotifiedImmediately expected:<1> but was:<0>
InternalConnectionTestCase.testConnectionStateReflectsClose                 expected:<true> but was:<false>

Run with only the two review fixes reverted, the tests covering them fail:

InternalConnectionTestCase.testSelfDeregisteringListenerDoesNotStopNotification ConcurrentModificationException
InternalConnectionTestCase.testListenerFailureDoesNotPreventServerClose         handleConnectionClosed(): wanted 1 time but was 0 times
ConnectionStateTest.testSelfDeregisteringListenerDuringClose                    ConcurrentModificationException
ConnectionStateTest.testSelfDeregisteringListenerDuringError                    ConcurrentModificationException

testRemovedListenerIsNotNotified passes both before and after — with the old code nobody was notified at all, so it is a characterisation test.

Regression runs across the modules that use internal connections or ConnectionState:

Module	Result
opendj-core	8171 tests, 0 failures
opendj-grizzly — GrizzlyLDAPConnection builds on ConnectionState	1038 tests, 0 failures
opendj-config — LDAPManagementContext builds on newInternalConnection	544 tests, 0 failures
opendj-rest2ldap	531 tests, 0 failures

[maximthomas]

Reusing ConnectionState here is the right call, and I reproduced every number in the description: InternalConnectionTestCase is 5/5 green on the branch and 4/5 red with the production file reverted (same lines, same messages); opendj-core 8166/0, opendj-config 544/0, opendj-rest2ldap 531/0. I also ran opendj-grizzly ConnectionFactoryTestCase (77 invocations, 0 failures) since it builds internal connections via Connections.newInternalConnectionFactory — worth adding to the matrix.

Two blocking issues, both confirmed by running the patched class against master.

Listener that de-registers itself breaks close (high)

ConnectionState iterates a LinkedList with a for-each while holding its own monitor, and removeConnectionEventListener is reentrant on that monitor. Driving Connections.newInternalConnection both ways:

Scenario	master	this PR
2 listeners, first removes itself	forwarded	2nd listener silently never notified, forwarded
4 listeners, first removes itself	forwarded	ConcurrentModificationException out of close(), 1 of 4 notified, server never told
…retry close()	forwards again	still never told — state is already CLOSED, unrecoverable

Self-deregistration on close is a normal idiom — opendj-core/src/main/java/org/forgerock/opendj/ldap/LDAPConnectionFactory.java:900 does exactly removeConnectionEventListener(this) inside its own handleConnectionClosed().

Two lines in opendj-core/src/main/java/org/forgerock/opendj/ldap/spi/ConnectionState.java fix it for every connection implementation, not just this one:

-import java.util.LinkedList;
 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
...
-    private final List<ConnectionEventListener> listeners = new LinkedList<>();
+    private final List<ConnectionEventListener> listeners = new CopyOnWriteArrayList<>();

Verified: 2/2 and 4/4 listeners notified, no CME, server told exactly once.

Listener exception skips the server-side close (high)

Same failure mode, simpler trigger. With this PR a RuntimeException from a listener propagates out of close() and serverConnection.handleConnectionClosed(...) is never called — on master the forward always happened. It escapes try-with-resources too, and that reaches production code: OpenDJProvider.newLDIFKeyStore builds a newInternalConnectionFactory, and opendj-core/src/main/java/org/forgerock/opendj/security/KeyStoreImpl.java closes those connections in six try (final Connection ...) blocks.

This matters more here than for LDAPConnectionFactory, where a separate connectionImpl.close() still runs. For an internal connection the forward is the only cleanup — RequestHandlerFactoryAdapter.doClose() is what cancels pending requests.

In opendj-core/src/main/java/org/forgerock/opendj/ldap/InternalConnection.java:

private final AtomicBoolean isClosed = new AtomicBoolean();

@Override
public void close(final UnbindRequest request, final String reason) {
    // Closing an already closed connection has no effect.
    if (isClosed.compareAndSet(false, true)) {
        try {
            state.notifyConnectionClosed();
        } finally {
            // A misbehaving listener must not prevent the server connection
            // from releasing its resources.
            serverConnection.handleConnectionClosed(messageID.getAndIncrement(), request);
        }
    }
}

Verified with both fixes applied: server told exactly once in every scenario, including the throwing-listener case. Baseline, 16-thread concurrent close (exactly 1 notification, 1 forward) and re-entrant close() from a listener all still behave. The exception still propagates — no notify site in the codebase guards individual callbacks, so that's the existing convention; the finally is the part that matters.

Nits

• Alert reference: #692 is accurate — alert 692 is java/unused-container at InternalConnection.java:52 — but GitHub auto-links it to issue #692, an unrelated closed import-ldif bug. "code-scanning alert #692" removes the trap.
• Pooling: no impact, in case it came up. I built a CachedConnectionPool over an internal connection factory and it behaves identically before/after — the pooled wrapper returns the delegate to the pool rather than closing it, so isValid() never flips.
• Stale javadoc: Connection.addConnectionEventListener (opendj-core/src/main/java/org/forgerock/opendj/ldap/Connection.java:287) still documents @throws IllegalStateException. Following the implementations is right, but testListenerAddedAfterCloseIsNotifiedImmediately now codifies the divergence — worth a follow-up to fix the javadoc.
• Missing tests: a self-deregistering-listener test and a throwing-listener test would each catch one of the issues above; a concurrent-close test would pin the idempotency claim. verifyClosedOnce(...) is asserted in only two of five tests.
• Loose matcher: any(UnbindRequest.class) passes even if a substituted request is forwarded; asserting the same instance is tighter.
• Pre-existing, not yours: an operation issued after close() blocks forever (jstack shows main parked in PromiseImpl.await), identical on master. Good argument for the deferred operation-level state checks — they'd turn an infinite hang into an IllegalStateException.

[vharseko]

Thanks — both blocking issues confirmed and fixed in 4f14465, along with every nit. Reproduced your findings before touching anything.

Listener that de-registers itself breaks close

Confirmed, and the root cause is where you point: ConnectionState sets state = CLOSED before iterating, so the exception escapes close() with the transition already done and the forward never reached — unrecoverable, exactly as your third row says.

One refinement to the table: the threshold is three, not four. LinkedList.ListItr.hasNext() only compares nextIndex < size and never checks modCount, so with two listeners the removal shrinks the list to exactly the cursor position and the loop just ends; from three on, next() runs checkForComodification() and throws:

2 listeners -> notified [L0] (no exception)
3 listeners -> notified [L0] THEN ConcurrentModificationException
4 listeners -> notified [L0] THEN ConcurrentModificationException

Applied your fix — LinkedList -> CopyOnWriteArrayList in ConnectionState. It covers all four notification loops (VALID.notifyConnectionClosed, ERROR.notifyConnectionClosed, notifyConnectionError, notifyUnsolicitedNotification) and the re-entrant add case as well: a listener registered from inside the loop is no longer a comodification, and since the state has already moved on it gets its immediate notification from the terminal-state branch rather than a second one from the snapshot.

Listener exception skips the server-side close

Confirmed and applied as you wrote it, AtomicBoolean plus finally. Agreed on leaving the exception propagating: guarding individual callbacks here would be the only such site in the SDK, and the finally is what actually protects RequestHandlerFactoryAdapter.doClose().

It does mean the closed-ness lives in two places now (isClosed and state), which I looked at twice — but the single-field alternatives are all worse: notifyConnectionClosed() cannot report the transition once it throws, and gating on state.isClosed() before the call reintroduces a race where two threads both forward.

Nits

• Alert reference — fixed, but note that code-scanning alert #692 still autolinks: GitHub picks up #692 regardless of the preceding words. The description now links the alert URL directly.
• Stale javadoc — fixed here rather than deferred. Checked all 12 implementations of addConnectionEventListener first: none throws IllegalStateException, so the @throws is gone from both Connection and ConnectionState (its own javadoc carried the same claim), replaced by a sentence saying the late listener is notified immediately.
• Missing tests — added all three: testSelfDeregisteringListenerDoesNotStopNotification (4 listeners, the first de-registering itself), testListenerFailureDoesNotPreventServerClose, testConcurrentCloseNotifiesAndForwardsOnce (16 threads on a latch). Plus testSelfDeregisteringListenerDuringClose and ...DuringError in ConnectionStateTest, since the fix belongs to that class. verifyClosedOnce(...) is now asserted in six of the eight tests.
• Loose matcher — tightened to same(request). Needed switching the tests to the two-argument close(request, reason), since AbstractConnection.close() builds its own unbind request; the loose overload survives only for the concurrency test and the try-with-resources-style paths.

Verification

With only the two fixes reverted, the four new tests fail and nothing else does:

InternalConnectionTestCase.testSelfDeregisteringListenerDoesNotStopNotification » ConcurrentModification
InternalConnectionTestCase.testListenerFailureDoesNotPreventServerClose:190->verifyClosedOnce:56
ConnectionStateTest.testSelfDeregisteringListenerDuringClose » ConcurrentModification
ConnectionStateTest.testSelfDeregisteringListenerDuringError » ConcurrentModification

With them applied: opendj-core 8171/0, opendj-grizzly 1038/0 (added to the matrix as you suggested — it is the other real user of ConnectionState), opendj-config 544/0, opendj-rest2ldap 531/0.

Agreed on leaving the operation-after-close hang alone: it is identical on master and belongs with the operation-level state checks.