[#137] Wait for cleanup in RequestDistributorTest.testFailedRequest

[vharseko]

Summary

Fixes the intermittent RequestDistributorTest.testSimpleRequest CI failure reported in #137 by closing the gap in testFailedRequest, the one test method in the class that did not wait for its own request to be unregistered before returning.

Root cause

RemoteRequest.getSendFunction() registers the map cleanup as a promise completion listener:

promise.thenOnResultOrException(new Runnable() {
    public void run() { completionCallback.complete(RemoteRequest.this); }
});

which is what runs remoteRequests.remove(request.getRequestId()) (RemoteConnectionGroup.allocateRequest).

In PromiseImpl.setState() the waiter is woken before the listener queue is drained:

synchronized (this) {
    state = newState;
    notifyAll();          // unblocks getOrThrowUninterruptibly()
}                          // monitor released here
while ((listener = listeners.poll()) != null) {
    handleCompletion(listener, newState);   // only now is the map entry removed
}

So returning from getOrThrowUninterruptibly() does not guarantee the entry has left remoteRequests. The window is normally microseconds, which is why this only shows up on a loaded CI runner.

testSimpleRequest (3 s sleep), testCallbackRequest, testBlockingCallbackRequest and testCancelRequest (5x1 s retry loops) all absorb that window before they return. testFailedRequest did not:

request.getPromise().getOrThrowUninterruptibly();
Assert.assertTrue(client.getRemoteRequests().isEmpty());   // unreachable
Assert.assertTrue(server.getLocalRequests().isEmpty());    // unreachable

The call throws RuntimeException("Unknown Test case number") (set via getExceptionHandler().handleException(...) in TestRemoteRequest.handleIncomingMessage, i.e. the HAS_EXCEPTION path), so both assertions were dead code and the method went straight to finally { connection.close(); } with no wait at all. Since every one of these methods only declares dependsOnMethods = { "testNoConnectionRequest" }, TestNG is free to schedule testFailedRequest right before testSimpleRequest or testCallbackRequest, whose entry assertion on client.getRemoteRequests().isEmpty() then races the pending listener — exactly the failure at line 147 in the report.

The server side is not affected: LocalRequest.handleException() calls removeRequest() synchronously before the error message is even sent, which is consistent with the CI log failing on getRemoteRequests() rather than getLocalRequests().

Change

testFailedRequest now catches the expected exception explicitly (so the expectedExceptions annotation is no longer needed and the assertions become reachable) and waits for remoteRequests/localRequests to drain with the same retry loop the sibling test methods use.

Test plan

• mvn -pl OpenICF-java-framework/connector-framework-rpc test -Dtest=RequestDistributorTest — 6/6 green
• 5 consecutive runs of the same command, all green
• The expected failure path is still asserted: Assert.fail("Not Failed") if no exception, plus a message check on the caught RuntimeException

Fixes #137