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

net: expose more socket options on TcpSocket #3082

Open
hawkw opened this issue Oct 31, 2020 · 15 comments
Open

net: expose more socket options on TcpSocket #3082

hawkw opened this issue Oct 31, 2020 · 15 comments
Assignees
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-net Module: tokio/net

Comments

@hawkw
Copy link
Member

hawkw commented Oct 31, 2020

Is your feature request related to a problem? Please describe.
While updating Hyper to use Tokio 0.3 (hyperium/hyper#2317), it turned out that most of the socket options that Hyper needs to set are no longer available on TcpStream or on the new TcpSocket type. Instead, it was necessary to to open the socket using socket2 and use socket2 to set the socket options, resulting in some fairly unpleasant code for converting between socket2::Socket and TcpSocket, and then performing an asynchronous connect via TcpSocket. It would be nice if TcpSocket supported these socket options directly.

In particular, it's not great to have to manually convert a socket2 Socket to a Tokio TcpSocket via IntoRawFd/FromRawFd. It's relatively easy for users to mess this up in the following ways:

  • Using AsRawFd rather than IntoRawFd, failing to transfer ownership of the file descriptor, so that dropping the Socket closes the file descriptor
  • Forgetting to set O_NONBLOCK before converting to TcpSocket. If the socket isn't put in non-blocking mode, using it will block, potentially causing the runtime to silently hang.
  • Missing platform support for Unix or for Windows, depending on what platform the user builds on. This is an easy mistake to make when locally built docs don't show the traits for other operating systems (e.g. a user on MacOS or Linux might miss the existence of IntoRawSocket/FromRawSocket traits on Windows).

Also, it requires unsafe code in downstream crates that would be nice to abstract over.

It would be nice if all of these operations could be performed on TcpSocket without having to also use socket2. Or, at least, it would be nice if the conversion between socket2 and TcpSocket was less complex.

Describe the solution you'd like
Ideally, TcpSocket would provide the ability to set at least the socket options required by Hyper:

  • TCP keepalive
  • send buffer size
  • receive buffer size

My understanding is that adding these options would require an upstream addition in mio, which I'm happy to make.

Describe alternatives you've considered

Alternatively, we could:

  • Leave things as they are, requiring users to use socket2 for setting any socket options other than SO_REUSEADDR. I discussed why this is not great above.

  • Rather than adding these additional options on TcpSocket, we could add a conversion between socket2::Socket and tokio::net::TcpSocket that abstracts over the complexities I discussed above. Adding a conversion would hide the unsafe code behind Tokio's API, ensure that the socket is put in non-blocking mode, and ensure that ownership of the file descriptor/Windows SOCKET is transferred.

    The downside of this is that it would require taking a socket2 dependency in Tokio, which may not be desirable for stability reasons. A dependency could be feature-flagged, though...

  • Add a From<T: IntoRawFd> or similar to TcpSocket that sets O_NONBLOCK etc. This is definitely a bad idea since there is no way to ensure that the file descriptor is actually a TCP socket. Let's not do that.

@hawkw hawkw added A-tokio Area: The main tokio crate M-net Module: tokio/net C-feature-request Category: A feature request. labels Oct 31, 2020
@hawkw hawkw self-assigned this Oct 31, 2020
@hawkw
Copy link
Member Author

hawkw commented Oct 31, 2020

It's worth noting that Mio's TcpSocket already exposes some additional socket options that Tokio doesn't expose:

We may want to expose these as well

@hawkw
Copy link
Member Author

hawkw commented Oct 31, 2020

Opened tokio-rs/mio#1384 to expose buffer size, and tokio-rs/mio#1385 to expose the keepalive interval. We may also want to add SO_NODELAY...

@Darksonn
Copy link
Contributor

Darksonn commented Nov 1, 2020

I think it makes total sense to add these methods to TcpSocket. 👍

hawkw added a commit to tokio-rs/mio that referenced this issue Nov 2, 2020
This commit adds methods for setting `SO_RECVBUF` and `SO_SNDBUF` to
Mio's `TcpSocket` type. It would be nice to eventually expose these in
Tokio, so adding them to Mio is the first step. 

See tokio-rs/tokio#3082 for details.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@Freaky
Copy link

Freaky commented Nov 26, 2020

I ran into a similar difficulty while updating tarssh to Tokio 0.3 - I was setting send/recv buffers on the TcpStream from an accept() call, but there seems no way to (sensibly) replicate this behaviour in 0.3?

psarna added a commit to psarna/tokio that referenced this issue Dec 14, 2020
In order to restore the ability to set keepalive duration
for a TCP stream, a pair of keepalive()/set_keepalive() functions
are implemented on top of mio's keepalive feature.

Refs tokio-rs#3082
psarna added a commit to psarna/tokio that referenced this issue Dec 14, 2020
In order to restore the ability to set keepalive duration
for a TCP stream, a pair of keepalive()/set_keepalive() functions
are implemented on top of mio's keepalive feature.

Refs tokio-rs#3082
psarna added a commit to psarna/tokio that referenced this issue Dec 14, 2020
In order to restore the ability to set keepalive duration
for a TCP stream, a pair of keepalive()/set_keepalive() functions
are implemented on top of mio's keepalive feature.

Refs tokio-rs#3082
@biluohc
Copy link
Contributor

biluohc commented Apr 23, 2021

So how to set TCP keepalive for socket from accept()?

@Darksonn
Copy link
Contributor

If Tokio does not expose a method for doing it, you can do it with the socket2 crate using something like SockRef.

@thedodd
Copy link

thedodd commented Jun 7, 2021

Having used these methods from tokio 0.2 — namely set_keepalive — it would be great to have these in tokio 1.0 as well. For now, I'm using SockRef as @Darksonn mentioned (thanks @Darksonn).

@esavier
Copy link

esavier commented Jun 8, 2021

Is there any reasoning why available socket options are not exposed?
This kind of scratches Tokio out from some application

@silence-coding
Copy link

silence-coding commented Aug 5, 2021

is there a better way to set keepalive? The way I'm doing it right now is inelegant.
#4028

@esavier
Copy link

esavier commented Aug 5, 2021

yeah, i have no idea why setting options is not in the tokio api, but currently this is only option that i am aware of. Basically you have to create external socket structure and hopefully cast it to the tokio's version after setting the correct option.

@wutianze
Copy link

wutianze commented May 5, 2022

BIND_DEVICE, please!!!

@howellzhu
Copy link

Two years have passed, any update on this issue please?
Noticed that @hawkw 's commit is closed: net: expose keepalive configuration on TcpSocket
But it seems no indirect solution provided, I mean, do we have to use non-safe methods?

@Darksonn
Copy link
Contributor

Darksonn commented Nov 8, 2022

The socket2 crate can generally set any socket option safely. Other than that, please open a new issue if there is some specific option that you think Tokio should provide.

@howellzhu
Copy link

@Darksonn thanks , following safe code can work:

use socket2::{SockRef, TcpKeepalive};
...
        info!("connecting to {addr}");
        let tcp = TcpStream::connect(&addr).await?;
        info!("tcp connected to {addr}");
        let ka = TcpKeepalive::new().with_time(std::time::Duration::from_secs(30));
        let sf = SockRef::from(&tcp);
        sf.set_tcp_keepalive(&ka)?;
...

@msdrigg
Copy link

msdrigg commented Dec 15, 2023

@howellzhu's code worked great for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-net Module: tokio/net
Projects
None yet
Development

Successfully merging a pull request may close this issue.