feat(server): support the client-output-buffer-limit configuration#3560
feat(server): support the client-output-buffer-limit configuration#3560git-hulk wants to merge 4 commits into
Conversation
737c793 to
bea7369
Compare
jihuayu
left a comment
jihuayu
left a comment
There was a problem hiding this comment.
The async close is not guaranteed to invoke Connection::OnWrite, because blocking commands replace the bufferevent callbacks.
For example, with:
client-output-buffer-limit pubsub 1m 0 0
a RESP3 client can run:
HELLO 3
SUBSCRIBE ch
BLPOP never-exists 0
and another client can publish an 8 MiB message to ch.
I reproduced this on this PR. BlockingCommander::StartBlocking() had replaced the write callback, so bufferevent_trigger(..., EV_WRITE, ...) invoked BlockingCommander::OnWrite() instead of Connection::OnWrite(). Since the list was still empty, it disabled EV_WRITE and returned without checking kCloseAsync.
After 12 seconds the client was still present in CLIENT LIST with:
cmd=blpop obuf=8388658
and client_output_buffer_limit_disconnections was already 1. The connection was only removed after the test client closed its socket. A non-blocked subscriber was closed normally in the control test.
cde23e2 to
2db424a
Compare
Add the client-output-buffer-limit configuration to disconnect clients
that are not reading data from the server fast enough, following the
same semantics as Redis: a client is disconnected immediately once the
hard limit is reached, or when it stays over the soft limit for more
than the configured seconds continuously.
Unlike Redis, all the classes are configured in a single line, and only
the specified classes are changed. The limits are disabled by default
for all classes of clients, e.g. to protect against slow Pub/Sub
subscribers with the same limits as Redis:
client-output-buffer-limit pubsub 32m 8m 60
The limit takes effect on the normal and pubsub classes since the
replication stream is written to the socket directly instead of going
through the connection output buffer. The slave class is accepted for
compatibility but takes no effect: slow replicas are still handled by
max-replication-lag and replication-send-timeout-ms.
The check runs every time data is appended to the connection output
buffer. A client over the limit is closed asynchronously by manually
triggering the deferred write callback, since the write callback of a
stuck client may never fire on its own; the callback then runs in the
owner worker's event loop where freeing the connection is safe.
Also expose the client_output_buffer_limit_disconnections counter in
INFO stats, and classify clients subscribed to only shard channels as
pubsub clients so that they are covered by the pubsub class as well.
Assistant-By Claude Fable 5
ce0a3c3 to
40cbd57
Compare
The asynchronous close of a connection relies on manually triggering the write callback, but blocking commands like BLPOP or XREAD replace the bufferevent callbacks while blocked, so the triggered callback landed in the blocking command's write callback which didn't check the close flags. A blocked client that exceeded the output buffer limit (or was killed by the CLIENT KILL command) was counted as disconnected but stayed alive until it was unblocked or closed by the peer. Check the close flags at the beginning of the blocking write callbacks, and unblock the keys and close the connection when it's scheduled to close, the same way as the EOF handling in BlockingCommander::OnEvent. Closing before consuming an element also prevents losing the element to the dropped output buffer when a wakeup races with the close. Assistant-By Claude Fable 5
@jihuayu Yes, good catch. |
|
There was a problem hiding this comment.
@git-hulk There are still several issues:
WAITalso replaces the bufferevent callback, but this case was missed in the current changes.- The soft limit is not checked proactively after it expires. We seem to be missing a cron task that actively closes the connection; instead, the connection is closed lazily. Is this intentional by design?
- The return value of the
PUBLISHcommand that triggers the connection closure is incorrect. The current PR returns0, while Redis 8.6.3 returns1. The message has already been added to the subscriber’s buffer, and the connection is only scheduled to be closed afterward, so the subscriber should still be counted as a receiver. - Redis does not require the soft limit to be lower than the hard limit. For example, Redis accepts
CONFIG SET client-output-buffer-limit "normal 1mb 2mb 10", while this PR rejects it.
|
@jihuayu I think we don't need to fix everything that pops up from AI reviews.
It's just a wait for the replication, and should be reasonable to keep it without checking the output buffer on every OnWrite.
Yes, the lazy behavior is intentional behavior.
I prefer to keep rejecting the config explicitly instead of silence-and-ignore in Redis. |
The PUBLISH command returned 0 when the published message caused the subscriber to exceed the output buffer limit, while Redis returns 1: the message was already appended to the output buffer of the subscriber before the connection was scheduled to close, so it should be counted as a receiver of the message. A connection that was already scheduled to close is also counted as a receiver since it remains subscribed until it's freed, the same as Redis, which counts a receiver per registered subscriber regardless of whether the reply was dropped by the scheduled close. Assistant-By Claude Fable 5
2 participants
Add the client-output-buffer-limit configuration to disconnect clients that are not reading data from the server fast enough, following the same semantics as Redis: a client is disconnected immediately once the hard limit is reached, or when it stays over the soft limit for more than the configured seconds continuously.
Unlike Redis, all the classes are configured in a single line, and only the specified classes are changed. The limits are disabled by default for all classes of clients, e.g. to protect against slow Pub/Sub subscribers with the same limits as Redis:
The limit takes effect on the normal and pubsub classes since the replication stream is written to the socket directly instead of going through the connection output buffer. The slave class is accepted for compatibility but takes no effect: slow replicas are still handled by max-replication-lag and replication-send-timeout-ms.
The check runs every time data is appended to the connection output buffer. A client over the limit is closed asynchronously by manually triggering the deferred write callback, since the write callback of a stuck client may never fire on its own; the callback then runs in the owner worker's event loop where freeing the connection is safe.
Also expose the client_output_buffer_limit_disconnections counter in INFO stats, and classify clients subscribed to only shard channels as pubsub clients so that they are covered by the pubsub class as well.
Part of #2284
Assistant-By Claude Fable 5