Found while writing the test for #123; unrelated to that change.
What happens
RemoteConnectionGroup.allocateRequest registers a RemoteRequest in remoteRequests before it is sent, and RemoteRequest.promise only comes into existence inside the send function (RemoteRequest.getSendFunction() → apply(holder)); it is also reset to null when the send throws, while trySendMessage moves on to the next socket of the group. So a registered request has getPromise() == null for the whole window between allocateRequest and a successful send.
WebSocketConnectionGroup.shutdown() cancels every registered request without checking that:
for (RemoteRequest<?, ?, WebSocketConnectionGroup, WebSocketConnectionHolder, RemoteOperationContext> remote : remoteRequests.values()) {
remote.getPromise().cancel(true); // WebSocketConnectionGroup.java:193
}
delegate.close();
RemoteConnectionGroup.submitRequestCancel (tmp.getPromise().cancel(true)) and RemoteRequest.cancel() (promise.cancel(false)) have the same unguarded access.
Observed
Closing a ClientRemoteConnectorInfoManager right after connect().getOrThrow(...) resolved, while its initial connector-info ControlRequest was still being sent (JDK 26, connector-server-grizzly tests):
java.lang.NullPointerException: Cannot invoke "org.forgerock.util.promise.Promise.cancel(boolean)" because the return value of "org.forgerock.openicf.common.rpc.RemoteRequest.getPromise()" is null
at org.forgerock.openicf.framework.remote.rpc.WebSocketConnectionGroup.shutdown(WebSocketConnectionGroup.java:193)
at org.forgerock.openicf.framework.remote.rpc.WebSocketConnectionGroup.principalIsShuttingDown(WebSocketConnectionGroup.java:175)
at org.forgerock.openicf.framework.client.ClientRemoteConnectorInfoManager.doClose(ClientRemoteConnectorInfoManager.java:268)
at org.forgerock.openicf.framework.remote.ConnectionPrincipal.close(ConnectionPrincipal.java:141)
Consequence
ConnectionPrincipal.close() flips isRunning to false first and then calls doClose(), so once the NPE escapes:
- the group's
delegate.close() is skipped and the remaining groups are never told the principal is shutting down;
- the manager's private
WebSocketConnectionHolders are not closed;
- the close listeners never run, so
ConnectionManager's registry.remove(info) does not happen and the dead manager stays registered for that RemoteWSFrameworkConnectionInfo.
A second close() is a no-op because isRunning is already false.
Reproduce
In connector-server-grizzly, with a ConnectorServer running: ConnectionManager.connect(info) → manager.connect().getOrThrow(30, SECONDS) → immediately manager.close(). It is timing dependent; it reproduced on the first attempt of #123's test before the test was changed to wait for findConnectorInfoAsync(...) to complete before closing.
Suggested fix
Treat a null promise as "nothing to cancel": skip it in shutdown() (and submitRequestCancel / RemoteRequest.cancel()), and remove the request from remoteRequests so a late send after shutdown does not register it again. RemoteRequest could alternatively create the promise in the constructor and only send in the send function, which would make getPromise() never null; that is a larger change touching the send-failure path (promise = null on exception).
Found while writing the test for #123; unrelated to that change.
What happens
RemoteConnectionGroup.allocateRequestregisters aRemoteRequestinremoteRequestsbefore it is sent, andRemoteRequest.promiseonly comes into existence inside the send function (RemoteRequest.getSendFunction()→apply(holder)); it is also reset tonullwhen the send throws, whiletrySendMessagemoves on to the next socket of the group. So a registered request hasgetPromise() == nullfor the whole window betweenallocateRequestand a successful send.WebSocketConnectionGroup.shutdown()cancels every registered request without checking that:RemoteConnectionGroup.submitRequestCancel(tmp.getPromise().cancel(true)) andRemoteRequest.cancel()(promise.cancel(false)) have the same unguarded access.Observed
Closing a
ClientRemoteConnectorInfoManagerright afterconnect().getOrThrow(...)resolved, while its initial connector-infoControlRequestwas still being sent (JDK 26,connector-server-grizzlytests):Consequence
ConnectionPrincipal.close()flipsisRunningtofalsefirst and then callsdoClose(), so once the NPE escapes:delegate.close()is skipped and the remaining groups are never told the principal is shutting down;WebSocketConnectionHolders are not closed;ConnectionManager'sregistry.remove(info)does not happen and the dead manager stays registered for thatRemoteWSFrameworkConnectionInfo.A second
close()is a no-op becauseisRunningis alreadyfalse.Reproduce
In
connector-server-grizzly, with aConnectorServerrunning:ConnectionManager.connect(info)→manager.connect().getOrThrow(30, SECONDS)→ immediatelymanager.close(). It is timing dependent; it reproduced on the first attempt of #123's test before the test was changed to wait forfindConnectorInfoAsync(...)to complete before closing.Suggested fix
Treat a
nullpromise as "nothing to cancel": skip it inshutdown()(andsubmitRequestCancel/RemoteRequest.cancel()), and remove the request fromremoteRequestsso a late send after shutdown does not register it again.RemoteRequestcould alternatively create the promise in the constructor and only send in the send function, which would makegetPromise()nevernull; that is a larger change touching the send-failure path (promise = nullon exception).