[Feature Request]: Relax error type in services

[ngryman]

Problem

Currently, services need to return a toy_rpc::Error which requires the user to write more code than necessary. The user needs to explicitly convert errors from external crates or implement the Into<toy_rpc::Error> trait when possible.

Here is an contrived example of what I'm talking about:

#[async_trait]
#[export_trait_impl]
impl Foo for FooService {
  async fn foo(&self, _: ()) -> toy_rpc::Result<()> {
    // manual conversation for external crates
    external_impl().map_err(|e| toy_rpc::Error::ExecutionError(e.to_string()))?;

    // needs an explicit `From` trait implementation to work (see below)
    foo_impl()?;

    Ok(())
  }
}

#[derive(Debug, thiserror::Error)]
enum FooError {
  #[error("unknown error")]
  Unknown,
}

// I need to implement this for all my errors
impl From<FooError> for toy_rpc::Error {
  fn from(e: FooError) -> Self {
    toy_rpc::Error::ExecutionError(e.to_string())
  }
}

fn foo_impl() -> Result<(), FooError> {
  Ok(())
}

It also seems that the variants of the toy_rpc::Error should be reserved for toy_rpc's internals. The only variant that I currently use is toy_rpc::Error::ExecutionError.

Proposition

If possible, it would be nice to relax the error type to Box<dyn std::error::Error + Send + Sync>. That would allow the user to leverage blanket implementations of Into<std::error::Error> that a lot of libraries seem to provide. It also would work out of the box for error management libraries such as anyhow and thiserror.

Internally, toy_rpc could wrap the user-provided error within a toy_rpc::Error::ExecutionError, so the rest of toy_rpc's internals wouldn't need to change anything.

For illustration purposes, the above code example would likely become:

#[async_trait]
#[export_trait_impl]
impl Foo for FooService {
  async fn foo(&self, _: ()) -> toy_rpc::Result<()> {
    // assuming external library support conversion to `std::error::Error`
    external_impl()?;

    // no need for an explicit `From` trait implementation
    foo_impl()?;

    Ok(())
  }
}

#[derive(Debug, thiserror::Error)]
enum FooError {
  #[error("unknown error")]
  Unknown,
}

fn foo_impl() -> Result<(), FooError> {
  Ok(())
}

Let me know your thoughts about this. If this is not a priority on your roadmap, I still can open a PR, but I would need some guidance 🙏

[minghuaw]

Good point. You could actually use anyhow already, it is feature gated behind “anyhow”. There was some discussions that can be found in #13.

I am thinking about improving error conversion, but I don't have a clear idea right now. One possibility I am considering is actually to separate the execution error and return Result<Result<ExecutionOk, ExecutionErr>, toy_rpc::Error>. If you place the client call in a function that returns anyhow::Result, then the client side usage will look like

let result: Result<ExecutionOk, ExecutionErr> = client.call("service.method", args).await?;

I am not sure if this is a good design or not. I guess for now maybe you can

1. take a look at whether #13 kind of addresses your need right now,
2. you could propose what you think is the ideal design to separate the server side execution error from other internal errors. (Some pseudo rust code for example)

That should be a good start point for improving error handling.

[minghuaw]

Specifically, #13 was mentioning the kind of usage showcased in the tokio_tcp example: service definition, service impl on server side, and client call

[minghuaw]

I will experiment with your proposition in the next few days. I am trying to unify the error with connection right now, so there may be other internal changes to the error conversion, and opening a PR would probably create a conflict.

[ngryman]

Oh, sorry, I didn't see #13. The current workaround with anyhow::Result does improve things, thanks 👍

---

For a longer-term solution, I can ramble a bit, at least to give you another user's perspective.

Ultimately I believe RPC should abstract away, as much as possible, any transport-related concept from the user. So the service definition and implementation should not require a specific return type. Concretely toy_rpc::Result shouldn't be visible to the server.

Here is an example of service definition that would be valid:

#[async_trait]
#[export_trait]
pub trait ExampleApi {
  #[export_method]
  async fn return_string(&self, _: ()) -> String;

  #[export_method]
  async fn return_integer(&self, _: ()) -> i32;

  #[export_method]
  async fn return_custom_error(&self, _: ()) -> anyhow::Result<()>;
}

The client, however, needs to be aware of any transport-related errors, like the current toy_rpc::Error already covers. So the generated client-stub should always return a toy_prc::Result. Now comes the question about how to through app-level errors into the mix.

I would tend to agree with your proposal and simply wrap whatever the underlying implementation returns in a toy_rpc::Result.

Here is what the above service definition would return on the client:

let example_api = client.example_api();

let res: toy_rpc::Result<String> = example_api.return_string().await;
let res: toy_rpc::Result<i32> = example_api.return_integer().await;
let res: toy_rpc::Result<anyhow::Result<()>> = example_api.return_custom_error().await;

A possible variant of the above API could be to flatten the Result to improve ergonomics. So if the implementation returns a Result, it would be unwrapped and injected in a toy_rpc::Result. The user error would be wrapped in a toy_rpc::Error variant, ExecutionError seems like a good existing candidate for that.

Here is an example to illustrate:

let res: toy_rpc::Result<()> = example_api.return_custom_error().await;
match res {
  Ok(()) => println!("The success value"),
  toy_rpc::Error::ExecutionError(e) => println!("App-level error"),
  _ => println!("All other errors"),
}

[minghuaw]

I like your proposition! Since it looks like this will introduce breaking changes, I will likely put this in version 0.9. I will keep you updated once I have some kind of proof of concept ready.

[minghuaw]

I have some initial implementations in the "0.9-devel" branch, which relaxes the return type from Result<_, _> to any type that is serializable. On the server/service definition side, the code like below would work

pub struct Foo {}

#[export_impl]
impl Foo {
  #[export_method]
  async fn echo(&self, args: String) -> String {
    args
  }
}

#[async_trait]
#[export_trait]
pub trait Bar {
  #[export_method]
  async fn is_bar(&self, args: ()) -> bool;
}

On the client side, it is almost like what you suggested

let echo: String = client.call("Foo.echo", "hello".to_string()).await?;

where a non-result return type Ret is mapped to the Ok type of the RPC response (Result<Ret, toy_rpc::Error>), and a return type of Result<Ok, Err> (including Result<_, _>, toy_rpc::Result<_>, or anyhow::Result<_>) is mapped to Result<Ok, toy_rpc::Error>.

However, there is a catch. The impl_for_client option on the #[export_trait] macro would require all methods to return Results (anyhow would probably be the preferred way in this case); however you will still be able to do things like

let echo: String = client.foo().echo("hello".into()).await?

So it is almost like what you proposed.

I have only run a limited set of tests so far. I will probably do more testing and write up some examples/documentation in the next few days. If everything goes well, I will do an alpha release on crates.io then.

[minghuaw]

I have released 0.9.0-alpha.1 on crates.io. I have added some documentation and examples for the preview release in the book. The particular chapter can be found here, and the tokio_tcp example in 0.9-devel channel has been updated to showcase the usage.

I will leave this issue open for further discussions until I guess formal release of 0.9.0.