chore: Remove async-trait #5812
Conversation
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`.
|
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. |
|
Hi @kayabaNerve, thank you for the PR! Could you share a bit about the motivation for removing the use of
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. |
|
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. |
| &mut self, | ||
| _: &StreamProtocol, | ||
| io: &mut T, | ||
| ) -> impl Send + Future<Output = io::Result<Self::Request>> |
There was a problem hiding this comment.
nit: it might be better to have Send last
There was a problem hiding this comment.
Additionally, we could probably leave the async signature intact here while having the trait member return impl Future<Output = T> + Send without a compiler warning. Thoughts on that?
There was a problem hiding this comment.
Sorry, I left a comment misunderstanding your note which I've now deleted.
async fn for the implementations should be possible 👍 TIL.
|
This pull request has merge conflicts. Could you please resolve them @kayabaNerve? 🙏 |
Would be one less dependency we would be using, and allow us to begin to adapt RPIT where possible.
Are we losing anything by removing |
|
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 ( |
|
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. |
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
Sendbounds follow the behavior from async-trait. async-trait's generation ofPin<Box<dyn Future>>also providedUnpinwhich has not been a preserved bound EXCEPT in the test code which required it in in-tree call sites. Moving forward, callers expectingUnpinshould wrap their futures themselves withBox::pin.Change checklist