-
Notifications
You must be signed in to change notification settings - Fork 964
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
*: Remove use of development_transport
in examples and tests
#3056
Conversation
These create circular dependencies to the root libp2p crate in tests.
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.
Hi Thomas, LGTM appart from the tcp::TcpTransport
-> tcp::async_io::Transport
and tokio
on the use-tokio
mdns
test
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.
LGTM!
MyBehaviour { | ||
rendezvous: rendezvous::client::Behaviour::new(identity.clone()), | ||
ping: ping::Behaviour::new(ping::Config::new().with_interval(Duration::from_secs(1))), | ||
}, | ||
PeerId::from(identity.public()), | ||
); | ||
) | ||
.executor(Box::new(|f| { |
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.
Curious Thomas, why do we need it here but not when using tcp::async_io::Tcp
?
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.
async-std
implicitly creates a reactor on each thread that an IO component is polled on. tokio only creates a reactor on worker threads and thus requires explicit control over where tasks are spawned.
By default, a Swarm
creates a futures::executor::Threadpool
for these background tasks and that doesn't work for tokio. Also see #3068.
just noticed https://github.com/libp2p/rust-libp2p/blob/master/src/tutorials/ping.rs also needs to be addressed |
How should we move forward here? Should I remove the deprecation and just not use those transports for the tests and crate-specific examples? That will break the circular dependencies which is what I am after. We can then decide separately, what we should rename those transport constructors to. @mxinden Input please :) |
development_transport
in examples and tests
Closing in favor of #3023. |
Description
These create circular dependencies to the root libp2p crate in tests.
Links to any relevant issues
libp2p-swarm-test
#2888: Once we have the shared test abstraction, more duplication in test setup will go away again. For the examples, I think we should not be using thedevelopment_transport
but be as close to a real-world application as possible.development_transport
inside our workspace.Open Questions
Change checklist
I have added tests that prove my fix is effective or that my feature works