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/{tcp,dns,websocket}: Remove Clone implementation for *Config #2682

Merged
merged 4 commits into from
May 31, 2022

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented May 30, 2022

Description

This commit removes the Clone implementation on GenTcpConfig and consequently the Clone
implementations on GenDnsConfig and WsConfig.

When port-reuse is enabled, GenTcpConfig tracks the addresses it is listening in a HashSet. This
HashSet is shared with the TcpListenStreams via an Arc<Mutex<_>>. Given that Clone is
derived on GenTcpConfig, cloning a GenTcpConfig, results in both instances sharing the same
set of listen addresses. This is not intuitive.

This behavior is for example error prone in the scenario where one wants to speak both plain DNS/TCP and
Websockets. Say a user creates the transport in the following way:

let transport = {
    let tcp = tcp::TcpConfig::new().nodelay(true).port_reuse(true);
    let dns_tcp = dns::DnsConfig::system(tcp).await?;
    let ws_dns_tcp = websocket::WsConfig::new(dns_tcp.clone());
    dns_tcp.or_transport(ws_dns_tcp)
};

Both dns_tcp and ws_dns_tcp share the set of listen addresses, given the dns_tcp.clone() to
create the ws_dns_tcp. Thus, with port-reuse, a Websocket dial might reuse a DNS/TCP listening
port instead of a Websocket listening port.

With this commit a user is forced to do the below, preventing the above error:

let transport = {
    let dns_tcp = dns::DnsConfig::system(tcp::TcpConfig::new().nodelay(true).port_reuse(true)).await?;
    let ws_dns_tcp = websocket::WsConfig::new(
        dns::DnsConfig::system(tcp::TcpConfig::new().nodelay(true).port_reuse(true)).await?,
    );
    dns_tcp.or_transport(ws_dns_tcp)
};

Links to any relevant issues

Follow up on #2670 //CC @Pj50

Open Questions

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

This commit removes the `Clone` implementation on `GenTcpConfig` and consequently the `Clone`
implementations on `GenDnsConfig` and `WsConfig`.

When port-reuse is enabled, `GenTcpConfig` tracks the addresses it is listening in a `HashSet`. This
`HashSet` is shared with the `TcpListenStream`s via an `Arc<Mutex<_>>`. Given that `Clone` is
`derive`d on `GenTcpConfig`, cloning a `GenTcpConfig`, results in both instances sharing the same
set of listen addresses. This is not intuitive.

This behavior is for example error prone in the scenario where one wants to speak both plain DNS/TCP and
Websockets. Say a user creates the transport in the following way:

``` Rust
let transport = {
    let tcp = tcp::TcpConfig::new().nodelay(true).port_reuse(true);
    let dns_tcp = dns::DnsConfig::system(tcp).await?;
    let ws_dns_tcp = websocket::WsConfig::new(dns_tcp.clone());
    dns_tcp.or_transport(ws_dns_tcp)
};
```

Both `dns_tcp` and `ws_dns_tcp` share the set of listen addresses, given the `dns_tcp.clone()` to
create the `ws_dns_tcp`. Thus, with port-reuse, a Websocket dial might reuse a DNS/TCP listening
port instead of a Websocket listening port.

With this commit a user is forced to do the below, preventing the above error:

``` Rust
let transport = {
    let dns_tcp = dns::DnsConfig::system(tcp::TcpConfig::new().nodelay(true).port_reuse(true)).await?;
    let ws_dns_tcp = websocket::WsConfig::new(
        dns::DnsConfig::system(tcp::TcpConfig::new().nodelay(true).port_reuse(true)).await?,
    );
    dns_tcp.or_transport(ws_dns_tcp)
};
```
@mxinden
Copy link
Member Author

mxinden commented May 30, 2022

Note that the above scenario is not very likely. libp2p-websocket is only available on the server which likely always favors TCP over Websocket to connect to other servers. Still, in case a remote advertises a WebSokcet listen address, the above error may occur.

@elenaf9
Copy link
Contributor

elenaf9 commented May 30, 2022

Related discussion: #2652 (comment).

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.

Makes sense to me!

transports/dns/CHANGELOG.md Outdated Show resolved Hide resolved
transports/tcp/CHANGELOG.md Outdated Show resolved Hide resolved
transports/websocket/CHANGELOG.md Outdated Show resolved Hide resolved
@mxinden mxinden merged commit 6078fc6 into libp2p:master May 31, 2022
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.

3 participants