`get_timeout` exceeds timeout about r2d2 HOT 11 OPEN

How does something like Hikari handle validity checks?

from r2d2.

During validation Hikari temporarily changes the network timeout of the connection here.

from r2d2.

Similarly, during validation a PostgresConnectionManager from r2d2-postgres could temporarily change the network timeout of its tls_connector. I will check whether the underlying types expose the necessary functionality.

from r2d2.

I will check whether the underlying types expose the necessary functionality.

Unfortunately, they do not. In r2d2-postgres the MakeTlsConnect wraps a tokio_postgres::Socket, which wraps a tokio::net::TcpStream, and neither has configurable timeouts.

from r2d2.

The Javadoc for Connection::setNetworkTimeout is kind of interesting in that it discusses the timeout only applying to reads and not writes. I guess in practice the outbound TCP buffer doesn't fill for a single query?

postgres could definitely grow support for read/write timeouts via tokio-postgres, though it is interesting to note that libpq does not offer that at all from what I can tell. I imagine they might just advise cranking the TCP keepalive interval down and relying on that.

from r2d2.

Thinking about this a bit more, it actually seems to me like TCP keepalive may be the better approach here compared to some kind of read/query timeout. It doesn't make assumptions about how long a query will legitimately take to run, and can detect a disconnect even while the connection is totally idle rather than only at the time of checkout.

from r2d2.

My understanding is TCP keepalives depend on three parameters:

• keepalive time, the amount of idle time before a keepalive
• keepalive interval, the amount of time after an unacknowledged keepalive before the next one
• keepalive probes, the number of unacknowledged keepalives before connection closure

tokio::net::TcpStream exposes set_keepalive(duration), which sets the connection’s keepalive time to duration. I think it does not allow configuration of keepalive interval and keepalive probes, whose UNIX defaults are 75 seconds and 9, which would still allow long timeouts.

An alternative is configuring the parameters at the machine level, but doing so could affect other services running on the machine, or the machine may be inaccessible.

I appreciate general read and write timeouts might introduce complexity and agree with your point about query time. Instead, what about limiting timeouts to validity checks that only make trivial queries? The Java interface exposes that functionality through isValid(timeout).

from r2d2.

We could definitely add a variant of is_valid that takes a timeout, but I'd want to be pretty careful about what timeouts are actually passed to it. For example, if get_timeout is called with a 10 second timeout and we're 9.99995 seconds in when a connection becomes available, we probably wouldn't want to try calling is_valid with a 0.00005 second timeout, since that would almost assuredly fail.

from r2d2.

Sounds good! I think is_valid(timeout) would ultimately require access to the Tokio runtime, so I will start with a pull request in the rust-postgres repository.

from r2d2.

Now that Rust-Postgres exposes is_valid(timeout), I can think of two options that would maintain backward compatibility:

• Let ManageConnection implementations bound is_valid() calls if they can or want. The bound would be independent of timeout in get_timeout(timeout).
• Add a method is_valid_timeout to the ManageConnection trait that calls is_valid by default. Then update get_timeout accordingly.

What do you think?

from r2d2.

I think adding ManageTimeout::is_valid_timeout with a default impl seems best.

from r2d2.