-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: remove async_std support in TCP crate #5955
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Left a couple of comments
async-io = ["dep:async-io", "if-watch/smol"] | ||
default = ["tokio"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should not have it default to tokio right now since it would require disabling default features to enable the needed feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will correct next commit
transports/tcp/src/lib.rs
Outdated
//! This crate provides a [`async_io::Transport`] (deprecated) and [`tokio::Transport`], | ||
//! which implement the [`libp2p_core::Transport`] trait for use as a | ||
//! transport with `libp2p-core` or `libp2p-swarm`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though the comment is modified, it is also duplicated from above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not soft deprecate and just hard deprecate? CC @elenaf9 @dariusc93
@gitToki initially did that, but I asked for a soft deprecation. I think a soft deprecation makes sense here because we might still have users that use Any specific reason why you'd prefer a direct removal @jxs? |
Would there be any reason to suddenly do a hard deprecation vs soft? Imo, I think we should do a soft (though I am not against a hard one) to give users a time to switch or figure out plans on migration (or supporting the executor themselves for the respected protocols and utils). |
Hi!
the reason is more work, double the PR's for something that has been declared as unmaintained, I feel redundant to have a version for users to remove their usage when they probably are already removing, added to the fact that I have seen soft deprecations lost in time. |
updated, should be good. @dariusc93 please let me know if everything look good to you |
@@ -47,8 +47,6 @@ use libp2p_core::{ | |||
multiaddr::{Multiaddr, Protocol}, | |||
transport::{DialOpts, ListenerId, PortUse, TransportError, TransportEvent}, | |||
}; | |||
#[cfg(feature = "async-io")] | |||
pub use provider::async_io; | |||
#[cfg(feature = "tokio")] | |||
pub use provider::tokio; | |||
use provider::{Incoming, Provider}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jxs @dariusc93 based on the discussion in #5935 (comment), should we make the Incoming
and Provider
traits public to allow a downstream implementation?
In the other affected crates (libp2p-quic
, libp2p-dns
, etc.) the Provider
traits are already public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we aren't really consistent on this. In libp2p-mdns
for example the Provider
trait is also private.
Still, I think we should make them public in all crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense to me, and possibly isolate them so they aren't duplicated right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense to me, and possibly isolate them so they aren't duplicated right?
Yes good idea. But let's do that in a follow-up PR.
This pull request has merge conflicts. Could you please resolve them @gitToki? 🙏 |
Is it good to you now @elenaf9 ? 🫡 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the follow-up @gitToki
|
||
use crate::{cbor::codec::Codec, Codec as _}; | ||
|
||
#[async_std::test] | ||
#[tokio::test] | ||
async fn test_codec() { | ||
let expected_request = TestRequest { | ||
payload: "test_payload".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the request-response tests to use tokio is out of scope for this PR but blocked by it. There is a pending PR #5857 for this, that is blocked by failing tests. Would be interested in helping on that PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure I will try to take a look
transports/websocket/src/lib.rs
Outdated
#[tokio::test] | ||
async fn dialer_connects_to_listener_ipv4() { | ||
let a = "/ip4/127.0.0.1/tcp/0/ws".parse().unwrap(); | ||
futures::executor::block_on(connect(a)) | ||
connect(a).await | ||
} | ||
|
||
#[test] | ||
fn dialer_connects_to_listener_ipv6() { | ||
#[tokio::test] | ||
async fn dialer_connects_to_listener_ipv6() { | ||
let a = "/ip6/::1/tcp/0/ws".parse().unwrap(); | ||
futures::executor::block_on(connect(a)) | ||
connect(a).await | ||
} | ||
|
||
fn new_ws_config() -> Config<tcp::async_io::Transport> { | ||
Config::new(tcp::async_io::Transport::new(tcp::Config::default())) | ||
fn new_ws_config() -> Config<tcp::tokio::Transport> { | ||
Config::new(tcp::tokio::Transport::new(tcp::Config::default())) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This should be a separate PR, and the current PR should only be merged afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you OK if I open a new PR for websocket ? I already made the change in a new branch (it was fast), available here: master...gitToki:rust-libp2p:websocket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks!
This pull request has merge conflicts. Could you please resolve them @gitToki? 🙏 |
migrate from async-std to tokio, asked before [here](#5955 (comment)) Pull-Request: #6054.
ref #5935
crate to update:
swarm, mDNS, and the transports TCP, QUIC and DNS.