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

Phase out async_trait macro #2684

Open
2 tasks done
holly-hacker opened this issue Dec 30, 2023 · 2 comments
Open
2 tasks done

Phase out async_trait macro #2684

holly-hacker opened this issue Dec 30, 2023 · 2 comments
Labels
accepted An accepted request or suggestion help wanted Contributions to this issue are needed request Request for new functionality
Milestone

Comments

@holly-hacker
Copy link

What's missing?

Rocket currently re-exports the async_trait crate for use with its Fairing (and possibly others). As of rust 1.75.0 this is no longer needed, therefore it would be nice to get rid of this dependency and use actual async functions for these traits.

This generally improves compile times, dependency count and IDE responsiveness.

Ideal Solution

For v0.6, remove async_trait (or put its usage behind a feature) and use async functions directly. This would raise the MSRV to 1.75.0.

Why can't this be implemented outside of Rocket?

This is an integral part of Rocket.

Are there workarounds usable today?

No. You bypass the need for the macro attribute by returning a Box<dyn Future<Output = T> + Send + 'a>, but this is unwieldy.

Alternative Solutions

No response

Additional Context

No response

System Checks

  • I do not believe that this feature can or should be implemented outside of Rocket.
  • I was unable to find a previous request for this feature.
@holly-hacker holly-hacker added the request Request for new functionality label Dec 30, 2023
@SergioBenitez
Copy link
Member

This is indeed the plan! The new interfaces for #1070 make use of async fn in traits. There's a new internal async_bound macro that lets us add Send bounds to the futures returned by the async functions as well. The idea is to transform all existing async traits into regular traits with async fns with the help of this macro. Once that lends, I or someone else can pick this task up, and it should be completable with relative ease.

@SergioBenitez SergioBenitez added the accepted An accepted request or suggestion label Jan 2, 2024
@SergioBenitez SergioBenitez added this to the 0.6.0 milestone Jan 2, 2024
@SergioBenitez SergioBenitez added the help wanted Contributions to this issue are needed label Jan 30, 2024
@SergioBenitez
Copy link
Member

SergioBenitez commented Jan 30, 2024

The async_bound macro is now in master and used by the new Listener trait's connect method:

pub trait Listener: Send + Sync {
type Accept: Send;
type Connection: Connection;
async fn accept(&self) -> io::Result<Self::Accept>;
#[crate::async_bound(Send)]
async fn connect(&self, accept: Self::Accept) -> io::Result<Self::Connection>;
fn socket_addr(&self) -> io::Result<Endpoint>;
}

I would love help in moving Rocket away from async_trait to raw async fn in traits, with the help of the async_bound macro. This would mean transitioning at least the following traits and all of their implementations in the core, contrib, and examples:

pub trait FromData<'r>: Sized;
pub trait IoHandler: Send;
pub trait Handler: Cloneable + Send + Sync + 'static;
pub trait FromFormField<'v>: Send + Sized;
pub trait Fairing: Send + Sync + Any + 'static;
pub trait FromRequest<'r>: Sized;
pub trait FromForm<'r>: Send + Sized;

The process would be to:

  1. Remove the #[async_trait] attribute from the declaration site.
  2. Remove the #[async_trait] attribute from implementation sites.
  3. Add #[async_bound(Send)] or #[async_bound(Send + Sync)] to trait methods as needed so that 1) the project compiles, and 2) we have the bounds we need. We want to restrict the returned futures a little as possible. As such, it would not be good to simply place all bounds on all methods unnecessarily. The compiler should help in figuring out which methods need which bounds.

Repeat this process once per trait. A commit per removal would be fantastic, finalizing the PR with a commit that removes the async_trait dependency entirely.

That's it! Any help would be appreciated.

Edit: we cannot make this change to traits that are object safe since doing so would make the trait not object safe. The following traits must be object safe, and thus (for the time being) must continue to use async_trait:

  • Fairing
  • IoHandler
  • Handler

Edit 2: it is possible that we can't migrate any of these traits due to rust-lang/rust#100013. I've run into an issue attempting to migrate FromRequest, for example, due to the lifetimes. I imagine we'll see the issue recur on the remaining viable traits due to the same lifetime choices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted An accepted request or suggestion help wanted Contributions to this issue are needed request Request for new functionality
Projects
Status: Backlog
Development

No branches or pull requests

2 participants