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

replace #[async_trait] with native async trait for trait Resolver #6751

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

AbnerZheng
Copy link
Contributor

@AbnerZheng AbnerZheng commented Feb 23, 2024

Refer to: #6657

The native async trait in Rust has a limitation where it requires explicit declaration if it implements the Send or non-Send version of a trait.

For instance, consider the following trait:

trait HttpService {
    async fn fetch(&self, url: Url) -> HtmlBody;
}

This desugars to:

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

By default, the returned Future does not have the Send bound, which is necessary for a multithreaded runtime. Without using trait_variant to create a multithreaded version of the trait, we encounter errors as below indicating that the future cannot be sent between threads safely.

error: future cannot be sent between threads safely
   --> crates/net/dns/src/query.rs:62:51
    |
62  |         self.queued_queries.push_back(Query::Root(Box::pin(resolve_root(resolver, link, timeout))))
    |                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future returned by `resolve_root` is not `Send`
    |
    = help: within `impl Future<Output = Result<(TreeRootEntry, LinkEntry<K>), (LookupError, LinkEntry<K>)>>`, the trait `Send` is not implemented for `impl Future<Output = std::option::Option<std::string::String>>`
note: future is not `Send` as it awaits another future which is not `Send`

For more details on this issue, refer to the following resources:
https://blog.rust-lang.org/inside-rust/2023/05/03/stabilizing-async-fn-in-trait.html#mvp-part-2-send-bounds-and-associated-return-types
https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#async-fn-in-public-traits

On the other hand, by utilizing the #[async_trait] attribute, async functions are transformed into methods that return Pin<Box<dyn Future + Send + 'async_trait>>. This results in a Future that is thread-safe by default.

@AbnerZheng
Copy link
Contributor Author

So it is beneficial to introduce #[trait_variant] to replace #[aync_trait]?

@AbnerZheng
Copy link
Contributor Author

By using -> impl trait, we can avoid using #[trait_variant]. For example we can rewrite the Resolver as below:

pub trait Resolver: Send + Sync + Unpin + 'static {
    /// Performs a textual lookup and returns the first text
    fn lookup_txt(&self, query: &str) -> impl Future<Output = Option<String>> + Send;
}

By adding bound Send manually, we can avoid the error mentioned above. And when implementing the trait Resolver, we can either use async fn or ->impl trait, both are ok.

impl<P: ConnectionProvider> Resolver for AsyncResolver<P> {
    async fn lookup_txt(&self, query: &str) -> Option<String> {
      ...
    }
}
impl<P: ConnectionProvider> Resolver for AsyncResolver<P> {
    fn lookup_txt(&self, query: &str) -> impl Future<Output=Option<String>> + Send {
        todo!()
    }
}

@AbnerZheng
Copy link
Contributor Author

WDYT? @mattsse

crates/net/dns/src/resolver.rs Outdated Show resolved Hide resolved
crates/net/dns/src/resolver.rs Outdated Show resolved Hide resolved
@mattsse mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Feb 23, 2024
@mattsse mattsse added this pull request to the merge queue Feb 23, 2024
Merged via the queue into paradigmxyz:main with commit 3003364 Feb 23, 2024
29 checks passed
@AbnerZheng AbnerZheng deleted the replace-async-trait branch February 23, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants