Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic exception is thrown when publishing a JetStream message #768

Open
joschiwald opened this issue Oct 13, 2022 · 6 comments
Open

Generic exception is thrown when publishing a JetStream message #768

joschiwald opened this issue Oct 13, 2022 · 6 comments

Comments

@joschiwald
Copy link

joschiwald commented Oct 13, 2022

Better identify JetStream connection and /communication errors.

Versions of io.nats:jnats and nats-server:

io.nats:jnats: 2.16.1
nats-server: 2.8.4

OS/Container environment:

Client: Windows 10
Server: alpine docker image

Steps or code to reproduce the issue:

  1. Start a nats-server with default configuration and no streams configured
  2. Publish a JetStream message
natsConn.jetStream().publish("test", myMessageBytes)

Expected result:

The publish function throws a JetStreamApiException with the right error code.

Actual result:

The publish function throws an IOException
https://github.com/nats-io/nats.java/blob/main/src/main/java/io/nats/client/impl/NatsJetStream.java#L153

@scottf
Copy link
Contributor

scottf commented Oct 19, 2022

For now you need to look at the getMessage() that came back in the IOException

Error Publishing: 503 No Responders Available For Request

Changing this would be a breaking change since JetStreamApiException is not an IOException

I'm changing this issue to an enhancement instead of a bug.

@joschiwald
Copy link
Author

This is my first approach to solve this issue. I'll test it next week when I have access to my development environment.

diff --git a/src/main/java/io/nats/client/api/ApiResponse.java b/src/main/java/io/nats/client/api/ApiResponse.java
index 5257cb4c..2353759f 100644
--- a/src/main/java/io/nats/client/api/ApiResponse.java
+++ b/src/main/java/io/nats/client/api/ApiResponse.java
@@ -31,7 +31,15 @@ public abstract class ApiResponse<T> {
     private final Error error;
 
     public ApiResponse(Message msg) {
-        this(new String(msg.getData(), UTF_8));
+        if (msg.isStatusMessage()) {
+            json = null;
+            error = Error.convert(msg.getStatus());
+            type = NO_TYPE;
+        } else {
+            json = new String(msg.getData(), UTF_8);
+            error = json == null ? null : Error.optionalInstance(json);
+            type = json == null ? NO_TYPE : JsonUtils.readString(json, TYPE_RE, NO_TYPE);
+        }
     }
 
     public ApiResponse(String json) {
diff --git a/src/main/java/io/nats/client/impl/NatsJetStream.java b/src/main/java/io/nats/client/impl/NatsJetStream.java
index d0145571..8e339cce 100644
--- a/src/main/java/io/nats/client/impl/NatsJetStream.java
+++ b/src/main/java/io/nats/client/impl/NatsJetStream.java
@@ -149,10 +149,6 @@ public class NatsJetStream extends NatsJetStreamImplBase implements JetStream {
     }
 
     private PublishAck processPublishResponse(Message resp, PublishOptions options) throws IOException, JetStreamApiException {
-        if (resp.isStatusMessage()) {
-            throw new IOException("Error Publishing: " + resp.getStatus().getCode() + " " + resp.getStatus().getMessage());
-        }
-
         PublishAck ack = new PublishAck(resp);
         String ackStream = ack.getStream();
         String pubStream = options == null ? null : options.getStream();

@scottf
Copy link
Contributor

scottf commented Oct 24, 2022

Yes, something like that would work, but again, we cannot change the type of exception thrown without a major version release as it would be a breaking change.

@scottf scottf changed the title Wrong Exception is thrown when publishing a JetStream message Generic exception is thrown when publishing a JetStream message Mar 18, 2023
@scottf
Copy link
Contributor

scottf commented Mar 18, 2023

Also see #858

@scottf
Copy link
Contributor

scottf commented Apr 11, 2023

@joschiwald 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?

@joschiwald
Copy link
Author

I like the idea. With that option we could remove the workaround with exception mapping from our code 😊I hope this is doable without having to add to much bloat to your codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants