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

proxy: replace net2 with socket2 #4891

Closed
olix0r opened this issue Aug 18, 2020 · 3 comments · Fixed by linkerd/linkerd2-proxy#635
Closed

proxy: replace net2 with socket2 #4891

olix0r opened this issue Aug 18, 2020 · 3 comments · Fixed by linkerd/linkerd2-proxy#635
Assignees

Comments

@olix0r
Copy link
Member

olix0r commented Aug 18, 2020

The proxy depends on the net2 crate, which has been deprecated and replaced by socket2. We should update our one or two uses:

linkerd/app/Cargo.toml
42:net2 = "0.2"

linkerd/app/integration/Cargo.toml
32:net2 = "0.2"

linkerd/app/integration/src/server.rs
205:        let listener = net2::TcpBuilder::new_v4().expect("Tcp::new_v4");

linkerd/app/integration/src/controller.rs
337:    let listener = net2::TcpBuilder::new_v4().expect("Tcp::new_v4");
@hawkw
Copy link
Contributor

hawkw commented Aug 24, 2020

As a side note, this isn't gonna be totally banished from our dependency tree until Tokio migrates from mio 0.6 to mio 0.7 (tokio-rs/tokio#2677) a breaking change scheduled for Tokio 0.3. But, I imagine that we can remove our uses of it (which all appear to be in the integration tests).

@hawkw
Copy link
Contributor

hawkw commented Aug 24, 2020

I actually have no idea why the integration tests don't just use tokio::net::TcpListener::bind...there is probably some backstory for this...

@hawkw
Copy link
Contributor

hawkw commented Aug 24, 2020

Ah, the test change from TcpListener::bind to socket2 was in #952, because we added a test that required introducing a delay between binding and listening on the socket. So, we do still need this, and can probably just switch to socket2.

hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Aug 24, 2020
The proxy's integration tests depend on the `net2` crate, which has been
deprecated and replaced by `socket2`. Since `net2` is no longer actively
maintained, `cargo audit` will warn us about it, so we should replace it
with `socket2`.

While I was making this change, I was curious why we were manually
constructing and binding these sockets at all, rather than just using
`tokio::net::TcpListener::bind`. After some archaeology, I determined
that this was added in linkerd/linkerd2#952, which added a test that
requires a delay between when a socket is _bound_ and when it starts
_listening_. `tokio::net::TcpListener::bind` (as well as the `std::net`
version) perform these operations together. Since this wasn't obvious
from the test code, I went ahead and moved the new `socket2` version of
this into a pair of functions, with comments explaining why we didn't
just use `tokio::net`.

Fixes linkerd/linkerd2#4891
hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Aug 24, 2020
The proxy's integration tests depend on the `net2` crate, which has been
deprecated and replaced by `socket2`. Since `net2` is no longer actively
maintained, `cargo audit` will warn us about it, so we should replace it
with `socket2`.

While I was making this change, I was curious why we were manually
constructing and binding these sockets at all, rather than just using
`tokio::net::TcpListener::bind`. After some archaeology, I determined
that this was added in linkerd/linkerd2#952, which added a test that
requires a delay between when a socket is _bound_ and when it starts
_listening_. `tokio::net::TcpListener::bind` (as well as the `std::net`
version) perform these operations together. Since this wasn't obvious
from the test code, I went ahead and moved the new `socket2` version of
this into a pair of functions, with comments explaining why we didn't
just use `tokio::net`.

Fixes linkerd/linkerd2#4891
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants