Skip to content
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

transports/dns: Remove Clone trait bound #2658

Merged
merged 1 commit into from May 19, 2022

Conversation

dignifiedquire
Copy link
Member

Description

Follow on to #2529, otherwise this error gets generated when trying to use it

let transport = libp2p::tcp::TokioTcpConfig::new().nodelay(true);
let transport = libp2p::websocket::WsConfig::new(transport.clone()).or_transport(transport);
let transport = libp2p::dns::TokioDnsConfig::system(transport).unwrap();

transport.upgrade(core::upgrade::Version::V1Lazy) // < this line fails
error[E0599]: the method `upgrade` exists for struct `GenDnsConfig<OrTransport<libp2p::libp2p_websocket::WsConfig<GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, trust_dns_resolver::name_server::connection_provider::GenericConnection, trust_dns_resolver::name_server::connection_provider::GenericConnectionProvider<trust_dns_resolver::name_server::connection_provider::tokio_runtime::TokioRuntime>>`, but its trait bounds were not satisfied
   --> iroh-p2p/src/service.rs:349:10
    |
349 |           .upgrade(core::upgrade::Version::V1Lazy)
    |            ^^^^^^^ method cannot be called on `GenDnsConfig<OrTransport<libp2p::libp2p_websocket::WsConfig<GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, trust_dns_resolver::name_server::connection_provider::GenericConnection, trust_dns_resolver::name_server::connection_provider::GenericConnectionProvider<trust_dns_resolver::name_server::connection_provider::tokio_runtime::TokioRuntime>>` due to unsatisfied trait bounds
    |
   ::: /Users/dignifiedquire/.cargo/git/checkouts/rust-libp2p-98135dbcf5b63918/999a212/core/src/transport/choice.rs:27:1
    |
27  |   pub struct OrTransport<A, B>(A, B);
    |   ----------------------------------- doesn't satisfy `_: Clone`
    |
   ::: /Users/dignifiedquire/.cargo/git/checkouts/rust-libp2p-98135dbcf5b63918/999a212/transports/dns/src/lib.rs:111:1
    |
111 | / pub struct GenDnsConfig<T, C, P>
112 | | where
113 | |     C: DnsHandle<Error = ResolveError>,
114 | |     P: ConnectionProvider<Conn = C>,
...   |
119 | |     resolver: AsyncResolver<C, P>,
120 | | }
    | |_- doesn't satisfy `_: libp2p::Transport`
    |
    = note: the following trait bounds were not satisfied:
            `OrTransport<libp2p::libp2p_websocket::WsConfig<GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>: Clone`
            which is required by `GenDnsConfig<OrTransport<libp2p::libp2p_websocket::WsConfig<GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, GenTcpConfig<libp2p::libp2p_tcp::tokio::Tcp>>, trust_dns_resolver::name_server::connection_provider::GenericConnection, trust_dns_resolver::name_server::connection_provider::GenericConnectionProvider<trust_dns_resolver::name_server::connection_provider::tokio_runtime::TokioRuntime>>: libp2p::Transport`

Change checklist

  • 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
  • A changelog entry has been made in the appropriate crates

dignifiedquire added a commit to n0-computer/iroh that referenced this pull request May 19, 2022
Depends on my fork, because it needs libp2p/rust-libp2p#2658
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!

@thomaseizinger thomaseizinger changed the title fix(dns): remove Clone boundary on Transport transports/dns: Remove Clone trait bound May 19, 2022
@thomaseizinger thomaseizinger merged commit d4c1292 into libp2p:master May 19, 2022
@elenaf9
Copy link
Contributor

elenaf9 commented May 19, 2022

CI failed on this PR merge due to a flaky AutoNAT test. I will look into it.

@elenaf9 elenaf9 mentioned this pull request May 19, 2022
1 task
@mxinden
Copy link
Member

mxinden commented May 22, 2022

Thanks @dignifiedquire for the fix. My bad.

We didn't catch this in CI, due to the fact that GenTcpConfig is Clone. I wonder whether we should continue having GenTcpConfig implement Clone. Especially once #2651 is fixed, a GenTcpConfig will contain shared mutable state. I wouldn't find it intuitive to be cloning a XXXConfig struct with the two structs then sharing some state.

@elenaf9
Copy link
Contributor

elenaf9 commented May 22, 2022

I wonder whether we should continue having GenTcpConfig implement Clone. Especially once #2651 is fixed, a GenTcpConfig will contain shared mutable state. I wouldn't find it intuitive to be cloning a XXXConfig struct with the two structs then sharing some state.

I think this will be solved with #2652 anyway. We add GenTcpTransport there as our TCP transport, which is not Clone anymore because it holds the Listeners streams. And GenTcpConfig will be just config flags.

@dignifiedquire dignifiedquire deleted the fix-dns-clone branch May 22, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants