mysql_async icon indicating copy to clipboard operation
mysql_async copied to clipboard

TCP keepalive definition may be better expressed in seconds

Open darnuria opened this issue 1 year ago • 2 comments

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. :)

darnuria avatar Aug 29 '23 14:08 darnuria

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

blackbeam avatar Aug 29 '23 15:08 blackbeam

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.

darnuria avatar Aug 30 '23 09:08 darnuria