[#124] Create the RemoteRequest promise at construction so an unsent request can be cancelled

[vharseko]

Fixes #124

Problem

RemoteConnectionGroup.allocateRequest registers a RemoteRequest in remoteRequests before it is sent, but RemoteRequest.promise only came into existence inside the send function (getSendFunction() → apply(holder)) and was reset to null when a send failed. For the whole window between registration and a successful send getPromise() returned null, and every cancellation path dereferenced it unguarded: WebSocketConnectionGroup.shutdown(), RemoteConnectionGroup.submitRequestCancel, RemoteRequest.cancel(), plus RemoteOperationRequest.check() through getExceptionHandler().

On the client the NullPointerException escaped ClientRemoteConnectorInfoManager.doClose() after ConnectionPrincipal.close() had already flipped isRunning, so the group's delegate.close(), the private WebSocket connections, globalConnectionGroups.remove and the close listeners (ConnectionManager's registry.remove(info)) were all skipped, and a second close() was a no-op. The server side has the same loop in OpenICFWebSocketApplication.close() / OpenICFWebSocketCreator.close(), where the NPE would stop the remaining groups from shutting down.

The window became reachable from user code with #104, which moved the initial CONNECTOR_INFO request from handshake() (before connectPromise resolves) to handshakeComplete() (after it): connect().getOrThrow() now returns while that request is registered but not yet sent, and close() from the caller's thread runs concurrently with the send.

Change

RemoteRequest only:

• The promise is created in the constructor and is final; getPromise() never returns null. The completion callback (removal from remoteRequests) is registered there too - PromiseImpl fires it on cancellation as well, so a request cancelled before it was sent unregisters itself.
• The send function does not send a request whose promise is already done. A request cancelled before the send hands its caller the cancelled promise (trySubmitRequest returns the request, getOrThrow throws the cancellation exception) instead of an answer that can never arrive.
• A failed send propagates without resetting anything; the group's trySendMessage retries on its next connection with the same promise.
• tryCancel(true) sends CancelOpRequest only for a delivered message, so a request cancelled before the send puts nothing on the wire. A cancel racing a send in progress is caught by a store-then-load pair on both sides (remoteCancelRequested / requestTime, deduplicated by remoteCancelSent): the cancel message is sent after the request, once, instead of ahead of it. If that late cancel message cannot be sent (the group's sockets are already gone), the delivered request is still reported as sent, with its promise cancelled.
• tryLock not acquired within a minute now throws IllegalStateException instead of returning an unsent promise.

Tests

RemoteRequestCancelBeforeSendTest (connector-framework-rpc), deterministic through a request that blocks in createMessageElement and a holder that blocks in sendString:

• promiseExistsWhileRequestIsRegisteredButNotSent, cancelBeforeSendCancelsPromiseWithoutSendingAnything (via submitRequestCancel), groupShutdownBeforeSendCancelsPromiseWithoutSendingAnything (the shutdown() loop) and cancelDuringSendNotifiesRemoteAfterTheRequestIsDelivered failed before the change - the first three with the exact NPE / null from the issue, the last one with the cancel message overtaking the request.
• concurrentCancelsAfterSendNotifyRemoteOnce: two concurrent cancel(true) after the send put one cancel message on the wire (pins remoteCancelSent); before the change each sent its own.
• cancelAfterSendNotifiesRemote, sendFailsOverToTheNextConnectionAndCompletesNormally and undeliverableCancelDuringSendKeepsTheDeliveredRequest (a cancel during the send whose tryCancelRemote throws: trySubmitRequest still returns the delivered request) pin down behaviour that had to survive the change; they pass before and after.

Local runs: connector-framework-rpc 21 tests, connector-framework-server 30, connector-server-grizzly 34, connector-server-jetty 34, all green.

Compatibility

getPromise() of a registered request is a pending promise before the send rather than null; a request cancelled before its send is returned by trySubmitRequest with a cancelled promise rather than sent. Both are visible only to code that raced the send, which previously got the NPE.

Noticed, not changed

• handshakeComplete() retries the initial CONNECTOR_INFO request on any failure, including the cancellation from shutdown(), and the retry goes out over the still-open socket before principalIsShuttingDown() removes it. Pre-existing and independent of this change: handshakeComplete() resends the initial CONNECTOR_INFO request while the group is shutting down #125, fixed in [#125] Do not resend the initial CONNECTOR_INFO request when shutdown() cancelled it #129.
• WebSocketConnectionGroup.principals is a plain TreeSet written from handshake threads and from principalIsShuttingDown() / close() without synchronisation.

[maximthomas]

praise: The promise now exists for as long as the request is registered, so every cancellation path has something to cancel.

• RemoteRequest.promise is final and built in the constructor together with the completion callback (RemoteRequest.java:59, :95), so a request cancelled before its send unregisters itself.
• apply() re-checks promise.isDone() under the lock (RemoteRequest.java:197), and tryCancel notifies the remote only for a delivered message (:83), so the cancel no longer overtakes the request.
• groupShutdownBeforeSendCancelsPromiseWithoutSendingAnything fails at the base, which pins the #124 NPE on the shutdown() loop itself.

---

suggestion (non-blocking): No test pins the isDone() re-check under the send lock.

OpenICF-java-framework/connector-framework-rpc/src/main/java/org/forgerock/openicf/common/rpc/RemoteRequest.java:187, :197

cancelBeforeSendCancelsPromiseWithoutSendingAnything and groupShutdownBeforeSendCancelsPromiseWithoutSendingAnything cancel while createMessageElement blocks, before apply() runs, so the fast path at :187 returns first. The re-check at :197 is what stops a cancel landing between :187 and tryLock. Changing :197 to !isSent() keeps connector-framework-rpc green (19/19). With :187 also reduced to isSent(), both cases fail with the request on the wire (sent.size() == 2).

            public Promise<V, E> apply(H remoteConnectionHolder) throws Exception {
                if (isSent()) {
                    return promise;
                }

Pin: with the fast path reduced to isSent(), the two cases above go red whenever the re-check at :197 is dropped (measured).

---

suggestion (non-blocking): No test pins remoteCancelSent, so a double CancelOpRequest passes the suite.

OpenICF-java-framework/connector-framework-rpc/src/main/java/org/forgerock/openicf/common/rpc/RemoteRequest.java:162-166, OpenICF-java-framework/connector-framework-rpc/src/test/java/org/forgerock/openicf/common/rpc/RemoteRequestCancelBeforeSendTest.java:129-173

Each case drives only one of the two callers of notifyRemoteCancelOnce() (:85 or :211). Replacing remoteCancelSent.compareAndSet(false, true) with true keeps connector-framework-rpc green (19/19). PromiseImpl.cancel checks isDone() without a lock, so two cancel(true) calls after the send both reach tryCancel, and that is enough to make the guard observable:

    @Test
    public void concurrentCancelsAfterSendNotifyRemoteOnce() throws Exception {
        RecordingHolder holder = new RecordingHolder(group, false);
        group.addConnection(holder);
        BlockingRequestFactory factory = new BlockingRequestFactory();
        factory.releaseSend();

        TestRemoteRequest<RecordingHolder> request = group.trySubmitRequest(factory);
        Assert.assertNotNull(request);
        holder.blockFirstSend(); // the next send - the first cancel message - blocks

        Future<Boolean> first = executor.submit(() -> request.getPromise().cancel(true));
        holder.awaitBlockedInSend();
        // The first cancel is still inside tryCancel: the promise is not done,
        // so this one reaches notifyRemoteCancelOnce() too.
        request.getPromise().cancel(true);
        holder.releaseSend();
        first.get(TIMEOUT_SECONDS, TimeUnit.SECONDS);

        Assert.assertTrue(request.getPromise().isCancelled());
        Assert.assertEquals(holder.sent.size(), 2, "request then one cancel: " + holder.sent);
    }

Pin: the true mutant sends a third message, which turns this case red. Not run.

---

question (non-blocking): Should a late cancel that cannot be delivered leave the send function's result alone and return the cancelled promise, as the tryCancel path does?

OpenICF-java-framework/connector-framework-rpc/src/main/java/org/forgerock/openicf/common/rpc/RemoteRequest.java:207-212, :84-88

WebSocketConnectionGroup.shutdown() can cancel(true) a request whose sender is still in sendBytes(...).get(), then close the group and drop its sockets. Once that send completes, notifyRemoteCancelOnce() at :211 runs outside any try. RemoteOperationRequest.trySendBytes finds no socket and throws ConnectorIOException. RemoteConnectionGroup.trySendMessage counts that as a failed connection and, with no other connection left, returns null. trySubmitRequest then deregisters the request and returns null, although the request was delivered and cancelled. The caller gets FAILED_EXCEPTION from AbstractAPIOperation.submitRequest, or ConnectionPrincipal.trySubmitRequest moves on to the next operational group. This is Minor as it stands, because it needs shutdown() to race an in-flight send. It would be Major if a deployment you support can resend a delivered request to another operational group this way.

                        requestTime = System.currentTimeMillis();
                        if (remoteCancelRequested) {
                            // Cancelled while the message was on its way:
                            // tryCancel saw it unsent. The promise is already
                            // cancelled; a cancel that cannot be sent must not
                            // fail the delivered request.
                            try {
                                notifyRemoteCancelOnce();
                            } catch (final Throwable ignored) {
                                // The transport is gone - nothing left to tell.
                            }
                        }

Pin: add a BlockingRequestFactory request whose tryCancelRemote throws, and cancel it with cancel(true) while holder.blockFirstSend() holds the send. At head trySubmitRequest returns null; with the fix it returns the request with a cancelled promise. Not run.

[vharseko]

@maximthomas all three are taken in 9ba0594, rebased onto current master (#128, #129).

1. isDone() re-check under the send lock. Done as you pinned it: the fast path is if (isSent()) return promise;, and the "cancelled before the send stays unsent" check now lives only under the lock. cancelBeforeSendCancelsPromiseWithoutSendingAnything and groupShutdownBeforeSendCancelsPromiseWithoutSendingAnything now go through it, and with the re-check reduced to !isSent() both fail with the request and a cancel on the wire (sent.size() == 2).

2. remoteCancelSent. Added concurrentCancelsAfterSendNotifyRemoteOnce as you wrote it. It passes, and with compareAndSet(false, true) replaced by true it fails with a second cancel message (sent.size() == 3).

3. Late cancel that cannot be delivered. Yes. This is a regression in this PR: the old code called tryCancelRemote only from tryCancel, inside a catch. The late notifyRemoteCancelOnce() in the send function is now wrapped the same way, so a delivered request keeps its result and the caller gets it back with a cancelled promise. Pinned by undeliverableCancelDuringSendKeepsTheDeliveredRequest (its tryCancelRemote throws). At the previous head trySubmitRequest returned null for it; now it returns the request. It passes against master too, so it sits with the tests that must survive the change.

Also added @Override to the four anonymous-class methods CodeQL flagged in this file.

connector-framework-rpc 21 tests, connector-framework-server 30 (with #129's WebSocketConnectionGroupShutdownTest), connector-server-grizzly 34, connector-server-jetty 34: all green locally.