feat(server): Support limiting the number of open connections. #1670
Conversation
src/server/server_family.cc
Outdated
| if (!res) | ||
| return false; | ||
|
|
||
| for (auto* listener : listeners_) { |
There was a problem hiding this comment.
It should only apply to the main port listener. For sure not for admin
There was a problem hiding this comment.
Changed (and added a test accordingly).
I added IsAdmin/SetAdmin methods to Listener, but I don't think it's an overkill, we already wanted something similar when we spoke about hiding admin connections from CLIENT LIST.
* Don't limit admin connections (and add a test case)
| bool IsAdminInterface() const; | ||
| void SetAdminInterface(bool is_admin = true); |
There was a problem hiding this comment.
We already have Connection::IsAdmin(), could we maybe consolidate them to use the same logic? (either inspect the port or explicitly set like here)
There was a problem hiding this comment.
Oooo, I did NOT expect the implementation of Connection::IsAdmin to be what it is 😆
Changed it to query the Listener.
| @@ -267,6 +275,10 @@ void Listener::OnConnectionClose(util::Connection* conn) { | |||
| } | |||
| } | |||
|
|
|||
| void Listener::OnMaxConnectionsReached(util::FiberSocketBase* sock) { | |||
| sock->Write(io::Buffer("-ERR max number of clients reached\r\n")); | |||
There was a problem hiding this comment.
How will this be handled by the client?
I imagine that it will receive an error before even sending a command, so will it even know how to treat it?
Also, don't we want to close the connection? (or is it implicitly done by the framework?)
There was a problem hiding this comment.
The clients I tested (python, redis-cli) seemed to handle it fine. redis does behave a bit differently (the socket doesn't close immediately and the message is delayed until the first command).
But then I tried to DDoS redis by opening a bunch of connections and leaving them open, and apparently it will start doing what we do.
and yeah, the framework is closing the connection :)
| @@ -399,8 +400,7 @@ uint32_t Connection::GetClientId() const { | |||
| } | |||
|
|
|||
| bool Connection::IsAdmin() const { | |||
| uint16_t admin_port = absl::GetFlag(FLAGS_admin_port); | |||
| return socket_->LocalEndpoint().port() == admin_port; | |||
| return static_cast<Listener*>(owner())->IsAdminInterface(); | |||
There was a problem hiding this comment.
Can we add IsAdminInterface() API to ListenerInterface?
I realize it's perhaps not clean, but having a downcast is messy and can lead to issues down the road
There was a problem hiding this comment.
IsAdminInterface is dragonfly specific. On the other side, facade::Connection is always issued by facade::Listener.
There was a problem hiding this comment.
I'm open to any suggestions of removing hardcoded downcasts :)
We could even template-ize Listener to have facade Listener return its own connection
There was a problem hiding this comment.
My personal opinion is that we should avoid casts if there is a reasonable design choice that allows it. And if not - we can have them and not feel bad about it. Specifically, any virtual inheritance implies the possibility of downcasts and it's fine.
There was a problem hiding this comment.
I don't like the cast either, if we start doing it too much I think we should find out a better way, but for now this was the shortest way I found to do this :)
* feat(server): Support limiting the number of open connections. * * Update helio after the small fix was merged to master * Don't limit admin connections (and add a test case) * Resolve CR comments
Close #1496.
Also, make
CONFIG GETuse config_registry so it's easier to add flags.