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

TCP keepalive definition may be better expressed in seconds #257

Open
darnuria opened this issue Aug 29, 2023 · 2 comments
Open

TCP keepalive definition may be better expressed in seconds #257

darnuria opened this issue Aug 29, 2023 · 2 comments

Comments

@darnuria
Copy link

Hello, found maybe a little mistake in how tcp_keepalive see OptsBuilder::tcp_keepalive() is defined in the lib. Thanks for the lib by the way.

By convention keepalive tcp parameters are seconds, in mysql_async it's currently defined from_millis().

https://github.com/blackbeam/mysql_async/blob/master/src/conn/mod.rs#L913

            } else {
                let keepalive = opts
                    .tcp_keepalive()
                    .map(|x| std::time::Duration::from_millis(x.into()));
                Stream::connect_tcp(opts.hostport_or_url(), keepalive).await?
            };

From socket2 lib source-code it's expected in seconds (not explicitly said in doc but readable in code).

#[allow(unused_variables)]
pub(crate) fn set_tcp_keepalive(fd: Socket, keepalive: &TcpKeepalive) -> io::Result<()> {
    #[cfg(not(any(target_os = "haiku", target_os = "openbsd", target_os = "nto")))]
    if let Some(time) = keepalive.time {
        let secs = into_secs(time);
        unsafe { setsockopt(fd, libc::IPPROTO_TCP, KEEPALIVE_TIME, secs)? }
    }

And From man 7 tcp man7.org tcp or linux.die.org man 7 tcp

TCP_KEEPCNT (since Linux 2.4)
    The maximum number of keepalive probes TCP should send before dropping the connection. This option should not be used in code intended to be portable. 
TCP_KEEPIDLE (since Linux 2.4)
    The time (in seconds) the connection needs to remain idle before TCP starts sending keepalive probes, if the socket option SO_KEEPALIVE has been set on this socket. This option should not be used in code intended to be portable. 
TCP_KEEPINTVL (since Linux 2.4)
    The time (in seconds) between individual keepalive probes. This option should not be used in code intended to be portable. 

Warning: it may be a breaking change for users who expected milliseconds.

If i am wrong don't hesitate to correct me. :)

@blackbeam
Copy link
Owner

Hi.
This is the reason, I believe. I'll consider changing this to seconds or at least adding a note in the docs.

@darnuria
Copy link
Author

darnuria commented Aug 30, 2023

Thanks for the reply, it's more clear!

(from tokio doc you linked)

If None is specified then keepalive messages are disabled, otherwise the number of milliseconds specified will be the time to remain idle before sending a TCP keepalive probe.

Some platforms specify this value in seconds, so sub-second millisecond specifications may be omitted.

Edit hit enter too soon.

Either documenting or change is fine for me, if you go for the doc I can do a PR today/tomorrow.

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

No branches or pull requests

2 participants