NatsRequestCompletableFuture throws CancellationException instead of TimeoutException on timeout

[ajax-semenov-y]

Hi. I just noticed that public CompletableFuture<Message> requestWithTimeout(String subject, byte[] body, Duration timeout) throws CancellationException when the timeout is exceeded. This code is located in NatsRequestCompletableFuture

public void cancelTimedOut() {
    wasCancelledTimedOut = true;
    completeExceptionally(new CancellationException("Future cancelled, response not registered in time, likely due to server disconnect."));
}

As for me, the more common exception for this case is TimeoutException. Is it possible to change CancellationException to TimeoutException?

[scottf]

@ajax-semenov-y
I agree with you that that exception isn't great. Plus the message is actually terrible, if not flat out wrong, it could simply be that there was no response within the timeout. I'm worried that it would be a breaking change though. I'm asking the team about it.

[scottf]

@ajax-semenov-y Actually...

If I throw a Timeout exception instead you end up getting an ExecutionException which has a TimeoutException as it's cause. I don't think this is any better.

[ajax-semenov-y]

@scottf, as for me that's ok to get ExecutionException with TimeoutException as its cause. At least it is consistent with the following code:

// kotlin code
CompletableFuture.runAsync { TimeUnit.SECONDS.sleep(5) }
        .orTimeout(1, TimeUnit.SECONDS)
        .get()

throws

Exception in thread "main" java.util.concurrent.ExecutionException: java.util.concurrent.TimeoutException
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2063)

also, in my case, it works well with the Project Reactor framework:

Mono.fromFuture {
        CompletableFuture.runAsync { TimeUnit.SECONDS.sleep(5) }
            .orTimeout(1, TimeUnit.SECONDS)
    }
        .doOnError(TimeoutException::class.java) {
            println("hello!")
        }
        .subscribe()

Such code prints "hello!" which means that the framework can parse TimeoutExcepton from the ExecutionException.

For now, I've created a workaround for this problem in my project with the following code (that's very ugly to rely on an exception message that's why I create this discussion):

Mono.fromFuture(connection.requestWithTimeout(subject, payload.toByteArray(), timeout))
    .onErrorResume(CancellationException::class.java) {
        // todo: this hack should be removed when
        // https://github.com/nats-io/nats.java/discussions/797 resolved
        if (it.message?.contains("response not registered in time") == true) {
            Mono.error(TimeoutException(it.message))
        } else {
            Mono.error(it)
        }
    }

[scottf]

The bigger problem is that it would be a breaking change in the API and I can't do that until we major version the client.

[ajax-semenov-y]

When do you plan to do the next major version increment?