Specialize errors #1081

nepalez · 2023-08-06T16:47:05Z

This is a continuation of the previous PR #1010

In this one I converted all crate-specific errors to specialized versions of the generic errors::Error<T>. Among other improvements I also converted PublishError, SubscribeError and UnsubscribeError into different variants of the same RequestError (which is crate::error::Error<RequestErrorKind>) to make it easier further improvement to return concrete errors instead of Box<dyn crate::Error> in jetstream and service

Jarema

Thank you for your PR!

It would be good to discuss refactor ideas in issue/discussions first to avoid reviews with such big number of comments :).

Jarema · 2023-08-09T06:45:51Z

async-nats/src/client.rs

@@ -149,15 +136,16 @@ impl Client {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn publish(&self, subject: String, payload: Bytes) -> Result<(), PublishError> {
+    pub async fn publish(&self, subject: String, payload: Bytes) -> Result<(), RequestError> {


One of the reasons for concrete errors was to only surface those ones, that are meaningful for the user.

After this change, we get irrelevant and maybe even misleading variants:

match client.publish("foo".into(), "data".into()).await { Ok(_) => (), Err(err) => match err.kind() { RequestErrorKind::TimedOut => todo!(), RequestErrorKind::NoResponders => todo!(), RequestErrorKind::Other => todo!(), RequestErrorKind::SendFlushFailed => todo!(), RequestErrorKind::FlushFailed => todo!(), RequestErrorKind::PublishingFailed => todo!(), RequestErrorKind::SubscriptionFailed => todo!(), RequestErrorKind::UnsubscriptionFailed => todo!(), }, }

We should stick to PublishError having variants valid only for Publish.

Publish never subscribes, not reach NoResponders etc.

Jarema · 2023-08-09T06:49:02Z

async-nats/src/client.rs

@@ -407,7 +398,7 @@ impl Client {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn subscribe(&self, subject: String) -> Result<Subscriber, SubscribeError> {
+    pub async fn subscribe(&self, subject: String) -> Result<Subscriber, RequestError> {


The same applies here.

Subscribe kind should not expose most of the variants in RequestError.

After all, Request is more or less subscribe + publish.

Jarema · 2023-08-09T06:49:25Z

async-nats/src/client.rs

@@ -473,16 +466,16 @@ impl Client {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn flush(&self) -> Result<(), FlushError> {


And the same applies here.

Jarema · 2023-08-09T06:57:23Z

async-nats/src/header.rs

@@ -44,6 +45,23 @@ use serde::{Deserialize, Serialize};
 /// # }
 /// ```

+#[derive(Clone, Debug, PartialEq)]


What is the reason behind aggregating those two errors?

My "final" target is to substitute the dynamic error crate::Error in public methods.

With this goal in mind, I looked at the code of nested methods where the error occurs. One of them is crate::jetstream::kv::entry which uses the private crate::Message::try_from which uses the private crate::stream::parse_headers.

This later beast parses both names and values and can return as dyn crate::Error both versions. That's why my first intention was to tell "these 2 errors are kinds of possible parsing-related problems that can be unioned".

But after you question I made a second look and recognized that in fact you're right, and my aggregation was a bit "premature" because the same error is raised not in parsing, but building context, where these 2 errors aren't necessarily related to each other.

That's why now I'm going to roll back here and instead of aggregating these 2 errors from the beginning, keep them separate, but provide the crate::stream::ParseError instead for all variants of the error that a parse_headers function should return. In that case aggregation should make more sense.

wdyt?

Jarema · 2023-08-09T09:33:11Z

async-nats/src/jetstream/consumer/pull.rs

+            RequestErrorKind::SubscriptionFailed => {
+                BatchError::with_source(BatchErrorKind::Subscribe, err)
+            }
+            _ => panic!("Cannot convert error"),


Here you can see that the irrelevant kinds of Error creates a possible panic scenario, or at least a path you need to handle in some way.

Jarema · 2023-08-09T09:36:30Z

async-nats/src/jetstream/message.rs

-                    std::io::ErrorKind::Other,
-                    "subscription dropped",
-                ))),
+                None => Err(AckErrorKind::SubscriptionDropped.into()),


Errors are aggregated into Other kind on purpose: to not clutter the possible enum variants with ones that it makes not much senes to match against for the user.

Jarema · 2023-08-09T09:39:15Z

async-nats/src/jetstream/message.rs

+
+pub type AckError = crate::error::Error<AckErrorKind>;
+
+#[derive(Clone, Copy, Debug, PartialEq)]


All those errors mean something really bad happened internally in NATS protocol.
There is no point for the user to consider those variants, and there is nothing they can do to fix it or prevent it.

Jarema · 2023-08-09T09:39:50Z

async-nats/src/lib.rs

@@ -928,13 +934,14 @@ impl Subscriber {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn unsubscribe(&mut self) -> Result<(), UnsubscribeError> {
+    pub async fn unsubscribe(&mut self) -> Result<(), RequestError> {


As pointed out before - most variants are not valid for unsubscribe.

Jarema · 2023-08-09T09:40:00Z

async-nats/src/lib.rs

@@ -965,27 +972,18 @@ impl Subscriber {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn unsubscribe_after(&mut self, unsub_after: u64) -> Result<(), UnsubscribeError> {
+    pub async fn unsubscribe_after(&mut self, unsub_after: u64) -> Result<(), RequestError> {


same here ;)

Jarema · 2023-08-09T09:41:55Z

async-nats/src/service/mod.rs

@@ -511,7 +512,7 @@ impl Service {
    /// # Ok(())
    /// # }
    /// ```
-    pub async fn endpoint<S: ToString>(&self, subject: S) -> Result<Endpoint, Error> {
+    pub async fn endpoint<S: ToString>(&self, subject: S) -> Result<Endpoint, RequestError> {


Maybe service API should have it's own Error. To consider.

nepalez · 2023-08-14T10:36:54Z

It would be good to discuss refactor ideas in issue/discussions first to avoid reviews with such big number of comments :).

Yeah, that's why it is draft ))

I'll think deeper about it. My first intention was simple -- to standardize errors for 2 reasons:

simplify the interface so that a user to get some crate::error::Error<T> in any case and could rely on this "contract" when debugging
get rid of the crate::Error alias type in public methods

But I see the reason in what you noticed above, so I'll need to rethink my approach.

Maybe it makes sense to step back and (instead of merging specific errors into variants of enum) provide a marker kind for them like pub enum FooKind {}.

PS. I will definitely split this PR into several smaller ones (another reason why this beast is draft -- it is more for the discussion of the whole picture, than for merging)

nepalez · 2023-09-22T12:49:19Z

I'm closing this as stale, maybe will return and review the task later

nepalez marked this pull request as draft August 7, 2023 19:37

nepalez force-pushed the specialize-errors branch from c511d9d to 8a21f78 Compare August 7, 2023 21:13

nepalez added 5 commits August 8, 2023 09:59

Convert AuthError to Error<AuthErrorKind>

c61c86d

Standardize ParseHeaderError

73b00af

Merge all client errors into RequestError

44e61dc

Make ParseOperationError a specialization of Error

e4f847a

Specialize errors from the service module

2f83fc1

nepalez force-pushed the specialize-errors branch from 8a21f78 to 2f83fc1 Compare August 8, 2023 08:05

Jarema requested changes Aug 9, 2023

View reviewed changes

This was referenced Aug 14, 2023

Convert AuthError to Error<AuthErrorKind> #1088

Open

Refactor ParseError #1089

Open

nepalez closed this Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specialize errors #1081

Specialize errors #1081

nepalez commented Aug 6, 2023

Jarema left a comment

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

nepalez Aug 14, 2023 •

edited

Loading

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

Jarema Aug 9, 2023

nepalez commented Aug 14, 2023 •

edited

Loading

nepalez commented Sep 22, 2023


		pub type AckError = crate::error::Error<AckErrorKind>;

		#[derive(Clone, Copy, Debug, PartialEq)]

Specialize errors #1081

Specialize errors #1081

Conversation

nepalez commented Aug 6, 2023

Jarema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nepalez Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nepalez commented Aug 14, 2023 • edited Loading

nepalez commented Sep 22, 2023

nepalez Aug 14, 2023 •

edited

Loading

nepalez commented Aug 14, 2023 •

edited

Loading