feat: implement connection timeout functionality #4407
Conversation
romange
force-pushed
the
Pr2
branch
4 times, most recently
from
f50e9dc to
c09c45c
Compare
`timeout` argument shuts down idle connections after the specified time. Fixes #1677 Signed-off-by: Roman Gershman <[email protected]>
|
|
||
| ABSL_FLAG(uint32_t, interpreter_per_thread, 10, "Lua interpreters per thread"); | ||
| ABSL_FLAG(uint32_t, timeout, 0, |
There was a problem hiding this comment.
maybe rename to connection_timeout?
There was a problem hiding this comment.
do you intend to change the default in the future?
There was a problem hiding this comment.
the name and the default value are compatible with valkey. see https://github.com/valkey-io/valkey/blob/unstable/valkey.conf#L161
| facade::Connection* dfly_conn = static_cast<facade::Connection*>(conn); | ||
| using Phase = facade::Connection::Phase; | ||
| auto phase = dfly_conn->phase(); | ||
| if ((phase == Phase::READ_SOCKET || dfly_conn->IsSending()) && |
There was a problem hiding this comment.
what is the reason to close the connection if its in READ_SOCKET phase?
From what I understand this will also close replication incoming connections if the replication is not on admin port
There was a problem hiding this comment.
closing idle connections in READ state is a desired behavior. You can check it with valkey by opening a connection with timeout=1 and run ping after a few seconds.
Having said that, good catch - we should exempt replication connections.
| void ServerState::ConnectionsWatcherFb(util::ListenerInterface* main) { | ||
| while (true) { | ||
| util::fb2::NoOpLock noop; | ||
| if (watcher_cv_.wait_for(noop, 1s, [this] { return gstate_ == GlobalState::SHUTTING_DOWN; })) { |
There was a problem hiding this comment.
running this every second can be heavy if server has many connections
There was a problem hiding this comment.
agree. that's why I added a TODO:
https://github.com/dragonflydb/dragonfly/pull/4407/files#diff-6032993b813fa531ddc738b01203e65ded0d630c65a5de559a6772b98fac5951R258
in fact, newest version of helio already allows partial traversal. I will fix the TODO in this PR.
timeoutargument shuts down idle connections after the specified time.Fixes #1677