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.
There was a problem hiding this comment.
Additionally, we could probably leave the
asyncsignature intact here while having the trait member returnimpl Future<Output = T> + Sendwithout a compiler warning. Thoughts on that?
You mean async fn read_request(...) -> impl Future<Output = ...>? What would be the advantage of that?
There was a problem hiding this comment.
I mean the traits would use fn read_request(...) -> impl Future<Output = ...> while the impl of the trait would use async fn read_request(...) -> ....
There was a problem hiding this comment.
Eg this patch should suffice (regarding request-response)
diff --git a/protocols/autonat/src/v1/protocol.rs b/protocols/autonat/src/v1/protocol.rs
index 6aa0c9916..f17a029d3 100644
--- a/protocols/autonat/src/v1/protocol.rs
+++ b/protocols/autonat/src/v1/protocol.rs
@@ -20,7 +20,6 @@
use std::io;
-use async_trait::async_trait;
use asynchronous_codec::{FramedRead, FramedWrite};
use futures::{
io::{AsyncRead, AsyncWrite},
@@ -39,7 +38,6 @@ pub const DEFAULT_PROTOCOL_NAME: StreamProtocol = StreamProtocol::new("/libp2p/a
#[derive(Clone)]
pub struct AutoNatCodec;
-#[async_trait]
impl request_response::Codec for AutoNatCodec {
type Protocol = StreamProtocol;
type Request = DialRequest;
diff --git a/protocols/rendezvous/src/codec.rs b/protocols/rendezvous/src/codec.rs
index 60f9f14f3..aec7c3c2c 100644
--- a/protocols/rendezvous/src/codec.rs
+++ b/protocols/rendezvous/src/codec.rs
@@ -20,7 +20,6 @@
use std::{fmt, io};
-use async_trait::async_trait;
use asynchronous_codec::{BytesMut, Decoder, Encoder, FramedRead, FramedWrite};
use futures::{AsyncRead, AsyncWrite, SinkExt, StreamExt};
use libp2p_core::{peer_record, signed_envelope, PeerRecord, SignedEnvelope};
@@ -241,7 +240,6 @@ impl Decoder for Codec {
#[derive(Clone, Default)]
pub struct Codec {}
-#[async_trait]
impl libp2p_request_response::Codec for Codec {
type Protocol = StreamProtocol;
type Request = Message;
diff --git a/protocols/request-response/src/cbor.rs b/protocols/request-response/src/cbor.rs
index 9dea02ef6..3b6f82c6d 100644
--- a/protocols/request-response/src/cbor.rs
+++ b/protocols/request-response/src/cbor.rs
@@ -49,7 +49,6 @@ pub type Behaviour<Req, Resp> = crate::Behaviour<codec::Codec<Req, Resp>>;
pub mod codec {
use std::{collections::TryReserveError, convert::Infallible, io, marker::PhantomData};
- use async_trait::async_trait;
use cbor4ii::core::error::DecodeError;
use futures::prelude::*;
use libp2p_swarm::StreamProtocol;
@@ -97,7 +96,6 @@ pub mod codec {
}
}
- #[async_trait]
impl<Req, Resp> crate::Codec for Codec<Req, Resp>
where
Req: Send + Serialize + DeserializeOwned,
diff --git a/protocols/request-response/src/codec.rs b/protocols/request-response/src/codec.rs
index d396a75ad..ac1ab06bf 100644
--- a/protocols/request-response/src/codec.rs
+++ b/protocols/request-response/src/codec.rs
@@ -19,14 +19,11 @@
// DEALINGS IN THE SOFTWARE.
use std::io;
-
-use async_trait::async_trait;
use futures::prelude::*;
/// A `Codec` defines the request and response types
/// for a request-response [`Behaviour`](crate::Behaviour) protocol or
/// protocol family and how they are encoded / decoded on an I/O stream.
-#[async_trait]
pub trait Codec {
/// The type of protocol(s) or protocol versions being negotiated.
type Protocol: AsRef<str> + Send + Clone;
@@ -37,43 +34,43 @@ pub trait Codec {
/// Reads a request from the given I/O stream according to the
/// negotiated protocol.
- async fn read_request<T>(
+ fn read_request<T>(
&mut self,
protocol: &Self::Protocol,
io: &mut T,
- ) -> io::Result<Self::Request>
+ ) -> impl Future<Output = io::Result<Self::Request>> + Send
where
T: AsyncRead + Unpin + Send;
/// Reads a response from the given I/O stream according to the
/// negotiated protocol.
- async fn read_response<T>(
+ fn read_response<T>(
&mut self,
protocol: &Self::Protocol,
io: &mut T,
- ) -> io::Result<Self::Response>
+ ) -> impl Future<Output = io::Result<Self::Response>> + Send
where
T: AsyncRead + Unpin + Send;
/// Writes a request to the given I/O stream according to the
/// negotiated protocol.
- async fn write_request<T>(
+ fn write_request<T>(
&mut self,
protocol: &Self::Protocol,
io: &mut T,
req: Self::Request,
- ) -> io::Result<()>
+ ) -> impl Future<Output = io::Result<()>> + Send
where
T: AsyncWrite + Unpin + Send;
/// Writes a response to the given I/O stream according to the
/// negotiated protocol.
- async fn write_response<T>(
+ fn write_response<T>(
&mut self,
protocol: &Self::Protocol,
io: &mut T,
res: Self::Response,
- ) -> io::Result<()>
+ ) -> impl Future<Output = io::Result<()>> + Send
where
T: AsyncWrite + Unpin + Send;
}
diff --git a/protocols/request-response/src/json.rs b/protocols/request-response/src/json.rs
index 5832c2b35..9a6ae496f 100644
--- a/protocols/request-response/src/json.rs
+++ b/protocols/request-response/src/json.rs
@@ -96,7 +96,6 @@ pub mod codec {
}
}
- #[async_trait]
impl<Req, Resp> crate::Codec for Codec<Req, Resp>
where
Req: Send + Serialize + DeserializeOwned,
There was a problem hiding this comment.
Oh nice! Didn't know that was possible. I thought the trait bounds always have to be the same between trait declaration and trait impl.
There was a problem hiding this comment.
The async sugar does produce equivalent bounds internally (or compilation errors if it can't). It's solely a difference in how they're writen.
|
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. |
The methods of Same for Moving from Wdyt of instead using
Even without support for dynamic dispatch I think we could use |
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