chore: Remove async-trait

[kayabaNerve]

Description

Removes async-trait for usage of RPIT.

Notes & open questions

libp2p's MSRV has included RPIT for a while now, making this possible. The Send bounds follow the behavior from async-trait. async-trait's generation of Pin<Box<dyn Future>> also provided Unpin which has not been a preserved bound EXCEPT in the test code which required it in in-tree call sites. Moving forward, callers expecting Unpin should wrap their futures themselves with Box::pin.

Change checklist

• [x] I have performed a self-review of my own code
• [ ] I have made corresponding changes to the documentation
• [ ] I have added tests that prove my fix is effective or that my feature works
• [x] A changelog entry has been made in the appropriate crates

[kayabaNerve]

libp2p-request-response already has an unpublished breaking change release in-tree. Most of the modified crates solely had their libp2p-request-response integrations edited, and already had a CHANGELOG entry for their update to the latest version.

libp2p-dns is weird. It has a hidden, public API item I did make breaking changes to and did not already have an unpublished breaking change in release (solely a patch release). It doesn't need a breaking change release if the hidden API entry is considered NOT part of the public API. I assumed that yes, there would be issues to change it without a new release however (as it has no in-tree users so it presumably services out-of-tree crates), and did bump the minor version. If these changes are an area of contest, they can be moved to a distinct PR as the libp2p-request-response changes are my priority.

No documentation/tests were needed for this, hence the unchecked boxes in the checklist.

It'd be great to have this included prior to the new releases which are about to occur. Sorry if it's too last minute.

[elenaf9]

Hi @kayabaNerve, thank you for the PR!

Could you share a bit about the motivation for removing the use of async-trait?

It'd be great to have this included prior to the new releases which are about to occur. Sorry if it's too last minute.

We are planning to release today, so unfortunately it's a bit too short notice. If it's very urgent we could cut another release soon-ish, but it be great to know why.

[kayabaNerve]

It's an unnecessary dependency with worse codegen? It only existed because RPIT wasn't stable and RPIT has been stable for months now. libp2p's MSRV includes RPIT as well, so there really is no blocker.

[mergify[bot]]

This pull request has merge conflicts. Could you please resolve them @kayabaNerve? 🙏

[dariusc93]

Could you share a bit about the motivation for removing the use of async-trait?

Would be one less dependency we would be using, and allow us to begin to adapt RPIT where possible.

It's an unnecessary dependency with worse codegen? It only existed because RPIT wasn't stable and RPIT has been stable for months now. libp2p's MSRV includes RPIT as well, so there really is no blocker.

Are we losing anything by removing async-trait that would be beneficial to rust-libp2p? I dont believe we are utilizing dynamic dispatch with async-trait (since it is not object safe right now for a trait member to utilize async signature or impl Trait to my knowledge) so that shouldnt be a problem. It would reduce some of the additional codegen, although I dont think the impact would be that much if we were to leave async-trait in place

[kayabaNerve]

The codegen arguments are definitely marginal. I don't expect this will be 10% on any benchmarks, or even 1%. Maybe a fraction of a percent on some benchmarks as I believe this does avoid a vtable lookup?

But the argument for this is to be in modern style, and for fine control of the exact bounds on returned futures.

I don't personally see anything lost but I do dread to hear this could be released and suddenly a non-trivial amount of downstream consumers do have to add glue to update (Box::pin calls on every returned future, for example). I don't expect that to be the case at all, I'm just deferring confirmation this won't be an issue to someone else.

[kayabaNerve]

trait-variant was supposed to be extended with support for dynamic dispatch over such traits. It wasn't and appears to be low priority/unmaintained.

https://github.com/spastorino/dynosaur does exist and would allow anyone who needs dynamic dispatch to continue their usage of it, but the actual derivation may need to be within libp2p... I'd advocate dropping support for dynamic dispatch or closing this PR before suggesting adopting a distinct shim crate at this level.

[elenaf9]

I don't personally see anything lost but I do dread to hear this could be released and suddenly a non-trivial amount of downstream consumers do have to add glue to update (Box::pin calls on every returned future, for example). I don't expect that to be the case at all, I'm just deferring confirmation this won't be an issue to someone else.

The methods of request_response::Codec are only called internally by the request-response behavior that wraps the Codec, so downstream user's shouldn't need to be concerned with handling the returned future. They do however need to change their implementation of Codec if they have a custom one, which in most cases will likely just mean wrapping the function body in an async statement.

Same for dns::Resolver.

---

Moving from async_trait to RPIT is fine by me. It's just a bit unfortunate that we now have to change the async fn signature to the (IMO a bit less ergonomic) impl Future + Send one.

Wdyt of instead using async fn with trait_variant::make, as suggested by async working group here? That way we wouldn't need to change the API.

trait-variant was supposed to be extended with support for dynamic dispatch over such traits. It wasn't and appears to be low priority/unmaintained.

Even without support for dynamic dispatch I think we could use trait-variant for the reason mentioned above.

[dariusc93]

Converting to draft to allow more discussion about this change

[dariusc93]

Thinking more on this, we could change it so it is similar to https://github.com/libp2p/rust-libp2p/pull/5812#discussion_r1947659149 where the trait itself would use fn -> impl Future while the impl would continue use async fn where possible since it would desugar to fn -> impl Future, which should allow us to avoid needing to use an async block or Box::pin unless its needed (as explained in the PR description). This would also allow us to reduce the diff and keeping it simple.

Looking at trait-variant, I dont think it really add much benefits here right now, in addition that it doesnt look like it is maintained (or at least no progress is made) so it would just be a near-dead dependency in our tree to where it would make more sense to continue using async-trait if actually needed

Thoughts @kayabaNerve @elenaf9

[elenaf9]

Thinking more on this, we could change it so it is similar to https://github.com/libp2p/rust-libp2p/pull/5812#discussion_r1947659149 where the trait itself would use fn -> impl Future while the impl would continue use async fn where possible since it would desugar to fn -> impl Future, which should allow us to avoid needing to use an async block or Box::pin unless its needed (as explained in the PR description). This would also allow us to reduce the diff and keeping it simple.

Sounds good. I didn't think this was possible because of the additional Send bound, but if that's not an issue then let's do that.