Use `async-trait` in `Service` trait ?

[benjamin-nw]

Hi,

I'm trying to call async fn's in call, but I run into issues.

I've a question for the Service trait in src/server/service.rs.

/// A Modbus server service.
pub trait Service {
    /// Requests handled by the service.
    type Request;

    /// Responses given by the service.
    type Response;

    /// Errors produced by the service.
    type Error;

    /// The future response value.
    type Future: Future<Output = Result<Self::Response, Self::Error>> + Send + Sync + Unpin;

    /// Process the request and return the response asynchronously.
    fn call(&self, req: Self::Request) -> Self::Future;
}

Is there a reason for not using async fn with async-trait ?

This could give us the ability to call .await in the call function.

This will change the trait like:

/// A Modbus server service.
#[async_trait]
pub trait Service {
    /// Requests handled by the service.
    type Request;

    /// Responses given by the service.
    type Response;

    /// Errors produced by the service.
    type Error;

    /// Process the request and return the response asynchronously.
    async fn call(&self, req: Self::Request) -> Result<Self::Response, Self::Error>;
}

[uklotzde]

async fn is just syntactic sugar, which would implicitly box the returned future. I didn't check how Rust 1.75 behaves in this case and if it is already supported.

Using a generic associated type saves a heap allocation. I am not deeply familiar with the server skeletons and how this design might complicate the implementation.

[benjamin-nw]

I didn't check how Rust 1.75 behaves in this case and if it is already supported.

By reading the blog, the new desugar is:

trait HttpService {
    async fn fetch(&self, url: Url) -> HtmlBody;
//  ^^^^^^^^ desugars to:
//  fn fetch(&self, url: Url) -> impl Future<Output = HtmlBody>;
}

---

I am not deeply familiar with the server skeletons and how this design might complicate the implementation.

Me neither 😆 , but I think that only having 1 function and 1 (or 2) GAT would simplify greatly the usage of the trait, and the implementation.

/// A Modbus server service.
pub trait Service {
    /// Errors produced by the service.
    type Error;

    /// The future response value.
    type Future: Future<Output = Result<Response, Self::Error>> + Send + Sync + Unpin;

    /// Process the `req` and return the [`Response`] asynchronously.
    fn call(&self, req: Request<'static>) -> Self::Future;
}

Or if you think that the new async or async_trait is usable:

with rust 1.75:

/// A Modbus server service.
#[trait_variant::make(Service: Send)]
pub trait LocalService {
    /// Errors produced by the service.
    type Error;

    /// Process the `req` and return the [`Response`] asynchronously.
    async fn call(&self, req: Request<'static>) -> Result<Response, Self::Error>;
}

with async_trait:

/// A Modbus server service.
#[async_trait]
pub trait Service {
    /// Errors produced by the service.
    type Error;

    /// Process the `req` and return the [`Response`] asynchronously.
    async fn call(&self, req: Request<'static>) -> Result<Response, Self::Error>;
}

[uklotzde]

Since the trait is supposed to be implemented only once in client code and not used for dynamic dispatch the non-boxed GAT version is more efficient.

If you need dynamic dispatch in your code you could probably do this behind a generic DynamicService trait and corresponding wrapper that simply delegates to your own, object safe trait.

[uklotzde]

Maybe related: #244

[uklotzde]

Would #245 fix this issue?

[benjamin-nw]

If you are going to do it like this:

fn call(
    &self,
    req: Self::Request,
) -> Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

We might just use async_trait because it desugar to a similar type but remove the boilerplate of having to add Box::pin(async move { // your code logic }) every time you implement the trait.

The original issue is to have an async block in order to able to call async fn in call.

I was thinking that maybe we can have 2 traits, one with async support and another without. But I saw that you want to change the project structure, so maybe this proposition can be in the mix too ?

[benjamin-nw]

Would https://github.com/slowtec/tokio-modbus/pull/245 fix this issue?

For me yes, but the user needs to always add : Box::pin(async move {}) in the impl.

But I'm not sure if it enables dynamic dispatch 🤔 because async_trait does other stuff that might help with that

[uklotzde]

async_trait doesn't work here because of the additional lifetime bound on &self. Try it and you will see. We can't add the required bound to the GAT in Service.

[benjamin-nw]

Oh, okay I see.

If we want to add async_trait we'll have to get rid of some of the GAT as I've written in my previous comment. But if you want to keep the GAT, I don't know if we can add it yeah.

[benjamin-nw]

The main issue is that it removes the Future GAT that is used pretty much everywhere in the server impl.

I need to try if I can use an async block with your #245 👍🏻 but I think it's possible now