Generic exception thrown from NatsJetStream.publishAsyncInternal()

[chrispoulsen]

Async errors are too generic

NatsJetStream:168 throws a RuntimeException in case an error happens. As this happens inside a future that RTE gets wrapped in a CompletionException before it re-emerges from the CompletableFuture as an error.

In my case I do the publishAsync() and want to ensure that the returned value / exception are of my types:

return js.publishAsync(natsMessage, options).handle(this::resultAndErrorMapper);

The mapper code looks like this currently:

    private LastPosition resultAndErrorMapper(PublishAck ack, Throwable throwable) {
        Throwable t = null;
        if(throwable instanceof CompletionException) {
            t = throwable.getCause().getCause();
        }
        if(t instanceof IOException e) {
            String msg = "Error, communicating with nats";
            LOG.error(msg, e);
            throw new CompletionException(new CommunicationError(msg, e));
        } else if(t instanceof JetStreamApiException jse) {
            if(jse.getApiErrorCode() == 10071) {
                String msg = "stream sequence does not match expected";
                LOG.debug(msg, jse);
                throw new CompletionException(new ExpectationNotMet(msg, jse));
            }
            String msg = "Bad request, data had errors";
            LOG.error(msg, jse);
            throw new CompletionException(new RuntimeException(msg, jse));
        }
       ...

It is the CompletionException.getCause().getCause() in the top, that seems out of order.

If the following patch is applied, the real exceptions are only wrapped in CompletionException (once) like CompletableFutures are supposed to work:

Index: src/main/java/io/nats/client/impl/NatsJetStream.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/io/nats/client/impl/NatsJetStream.java b/src/main/java/io/nats/client/impl/NatsJetStream.java
--- a/src/main/java/io/nats/client/impl/NatsJetStream.java	(revision 21b4fdcd84f372f8ddccdfdae59f7ed6acdf9c63)
+++ b/src/main/java/io/nats/client/impl/NatsJetStream.java	(date 1678911091349)
@@ -22,6 +22,7 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
 
 import static io.nats.client.support.NatsJetStreamClientError.*;
 import static io.nats.client.support.Validator.*;
@@ -165,7 +166,7 @@
                 responseRequired(resp);
                 return CompletableFuture.completedFuture(processPublishResponse(resp, options));
             } catch (IOException | JetStreamApiException e) {
-                throw new RuntimeException(e);
+                throw new CompletionException(e);
             }
         });
     }

In Java v9+ the "throws" line would probably be replaced with the following, to better express the intent:

                 responseRequired(resp);
                 return CompletableFuture.completedFuture(processPublishResponse(resp, options));
             } catch (IOException | JetStreamApiException e) {
-                throw new RuntimeException(e);
+                return CompletableFuture.failedFuture(e);
             }
         });
     }

Basically it feels like the CompletionException( new RuntimeException( IOException | JetStreamApiException ) ) structure is wrong.

[scottf]

Also see #768

[cpoulsen-dezide]

I did see the other issue, my understanding was that it talked about IOException vs. JetStreamAPIException - This one is not about that. No matter which of those two is thrown here, it will always be wrapped in new RuntimeException - dumbing down the return type from the API needlessly (and afterwards CF will wrap it in a CompletionException) - When you resolve one of these async operations, you get either the success result or the runtime exception wrapping one of the two actual exceptions (assuming that you started by "unpacking" the CompletionException which you always must do, as that is part of the contract for working with CF).

CompletableFuture use CompletionException as the means to carry checked exceptions around (and will wrap anything that is not a completion exception into one, so explicitly wrapping the NATS API exceptions that are the direct error indicators everywhere else in an RTE before having CF wrap it again, seems like an error.

The other methods in the client expose the error types directly, so this one stands out - Especially because there is no technical reason for wrapping the exceptions an extra time.

[scottf]

@chrispoulsen Yes, I just linked them, they are similar. I haven't had time to fully grok this, but when I do, I will have to make sure any change is backaward compatible, meaning we might have to live with this until a major version change.

[scottf]

@cpoulsen-dezide I'm thinking about how to fix this in a backward compatible manner. I could add a connection option that is essentially "better error messages" and behave differently if the user sets this option. What do you think?

[chrispoulsen]

@cpoulsen-dezide I'm thinking about how to fix this in a backward compatible manner. I could add a connection option that is essentially "better error messages" and behave differently if the user sets this option. What do you think?

If flipping that option on would result in a more correct API - That in turn would make my code have a shape that translates better with the next major (breaking) update of the client, then I would use it.

From an API perspective it might be a bit dangerous to start locking down the shape of next API version- I guess it depends on how mature everything is in the client API and when an update is expected.

This issue is not about a deal-breaker - it was more about it triggering my OCD as everything is usable, it is just not as elegant as it could be.