You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "git-hulk (via GitHub)" <gi...@apache.org> on 2026/07/14 11:27:18 UTC
[PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
git-hulk opened a new pull request, #3560:
URL: https://github.com/apache/kvrocks/pull/3560
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:
```
client-output-buffer-limit normal 0 0 0 slave 0 0 0 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.
Part of #2284
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#discussion_r3607359658
##########
src/config/config.cc:
##########
@@ -444,6 +446,47 @@ void Config::initFieldValidator() {
// The callback function would be invoked after the field was set,
// it may change related fields or re-format the field. for example,
// when the 'dir' was set, the db-dir or backup-dir should be reset as well.
+// Parses a client-output-buffer-limit spec like "normal 0 0 0 pubsub 32m 8m 60"
+// and applies it to the limits of the specified client kinds. The full spec is
+// parsed before applying anything, so a malformed quadruple cannot leave the
+// limits partially updated.
+Status Config::parseClientOutputBufferLimits(const std::string &v) {
+ std::vector<std::string> args = util::Split(v, " \t");
+ if (args.empty() || args.size() % 4 != 0) {
+ return {Status::NotOK, "should be in the format of <class> <hard limit> <soft limit> <soft seconds> ..."};
+ }
+
+ struct ParsedLimit {
+ ClientKind kind;
+ uint64_t hard_limit_bytes, soft_limit_bytes, soft_limit_seconds;
+ };
+ std::vector<ParsedLimit> parsed;
+ for (size_t i = 0; i < args.size(); i += 4) {
+ ClientKind kind = ClientKind::kNormal;
+ if (util::EqualICase(args[i], "normal")) {
+ kind = ClientKind::kNormal;
+ } else if (util::EqualICase(args[i], "slave") || util::EqualICase(args[i], "replica")) {
+ kind = ClientKind::kSlave;
+ } else if (util::EqualICase(args[i], "pubsub")) {
+ kind = ClientKind::kPubsub;
+ } else {
+ return {Status::NotOK, fmt::format("unknown client kind '{}'", args[i])};
+ }
+ auto hard = GET_OR_RET(ParseSizeAndUnit(args[i + 1]).Prefixed("invalid hard limit"));
Review Comment:
Yes, but we don't handle this in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on code in PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#discussion_r3583906452
##########
src/config/config.cc:
##########
@@ -444,6 +446,47 @@ void Config::initFieldValidator() {
// The callback function would be invoked after the field was set,
// it may change related fields or re-format the field. for example,
// when the 'dir' was set, the db-dir or backup-dir should be reset as well.
+// Parses a client-output-buffer-limit spec like "normal 0 0 0 pubsub 32m 8m 60"
+// and applies it to the limits of the specified client kinds. The full spec is
+// parsed before applying anything, so a malformed quadruple cannot leave the
+// limits partially updated.
+Status Config::parseClientOutputBufferLimits(const std::string &v) {
+ std::vector<std::string> args = util::Split(v, " \t");
+ if (args.empty() || args.size() % 4 != 0) {
+ return {Status::NotOK, "should be in the format of <class> <hard limit> <soft limit> <soft seconds> ..."};
+ }
+
+ struct ParsedLimit {
+ ClientKind kind;
+ uint64_t hard_limit_bytes, soft_limit_bytes, soft_limit_seconds;
+ };
+ std::vector<ParsedLimit> parsed;
+ for (size_t i = 0; i < args.size(); i += 4) {
+ ClientKind kind = ClientKind::kNormal;
+ if (util::EqualICase(args[i], "normal")) {
+ kind = ClientKind::kNormal;
+ } else if (util::EqualICase(args[i], "slave") || util::EqualICase(args[i], "replica")) {
+ kind = ClientKind::kSlave;
+ } else if (util::EqualICase(args[i], "pubsub")) {
+ kind = ClientKind::kPubsub;
+ } else {
+ return {Status::NotOK, fmt::format("unknown client kind '{}'", args[i])};
+ }
+ auto hard = GET_OR_RET(ParseSizeAndUnit(args[i + 1]).Prefixed("invalid hard limit"));
Review Comment:
It looks a little different from Redis: https://github.com/redis/redis/blob/e5e1eaa97ad3020305b84442197011abc8ce40bc/redis.conf#L8-L18
##########
src/server/redis_connection.cc:
##########
@@ -93,11 +93,60 @@ std::string Connection::ToString() {
evbuffer_get_length(Input()), evbuffer_get_length(Output()), last_cmd_, set_info_.lib_name, set_info_.lib_ver);
}
-void Connection::Close() {
+void Connection::Close(bool is_async) {
+ if (is_async) {
+ // Only the first caller should schedule the close since concurrent reply
+ // paths (e.g. publishers on other workers) may race here.
+ if (flags_.fetch_or(kCloseAsync) & kCloseAsync) return;
+
+ // The write callback of a stuck client may never be invoked since its output
+ // buffer cannot drain, so trigger the callback manually instead of waiting
+ // for it. Ignoring watermarks is required because the write callback is only
+ // triggered when the output buffer size is not larger than the low watermark.
+ // The callback is deferred(BEV_OPT_DEFER_CALLBACKS) and runs in the owner
+ // worker's event loop, where it's safe to free the connection.
+ bufferevent_trigger(bev_, EV_WRITE, BEV_TRIG_IGNORE_WATERMARKS | BEV_TRIG_DEFER_CALLBACKS);
+ return;
+ }
+
if (close_cb) close_cb(GetFD());
owner_->FreeConnection(this);
}
+bool Connection::IsExceedOutputBufferLimit() {
+ // Connections that are already scheduled to close don't need to be checked
+ // again. The replication stream is written to the socket directly instead of
+ // going through the connection output buffer, so the slave kind is not
+ // applicable here: slow replicas are handled by max-replication-lag and
+ // replication-send-timeout-ms.
+ if (IsFlagEnabled(kCloseAsync) || IsFlagEnabled(kCloseAfterReply) || IsFlagEnabled(kSlave)) return false;
+
+ auto kind = GetClientType() == kTypePubsub ? ClientKind::kPubsub : ClientKind::kNormal;
+ const auto &limit = srv_->GetConfig()->GetClientOutputBufferLimit(kind);
+ uint64_t hard_limit_bytes = limit.hard_limit_bytes.load(std::memory_order_relaxed);
+ uint64_t soft_limit_bytes = limit.soft_limit_bytes.load(std::memory_order_relaxed);
+ if (hard_limit_bytes == 0 && soft_limit_bytes == 0) return false;
+
+ uint64_t used_bytes = evbuffer_get_length(Output());
+ if (hard_limit_bytes != 0 && used_bytes >= hard_limit_bytes) return true;
+
+ if (soft_limit_bytes != 0) {
+ if (used_bytes >= soft_limit_bytes) {
+ auto soft_limit_seconds = static_cast<int64_t>(limit.soft_limit_seconds.load(std::memory_order_relaxed));
Review Comment:
Can we use the same type for limit.soft_limit_seconds and soft_limit_seconds to avoid overflow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-5010624001
> 1. `WAIT` also replaces the bufferevent callback, but this case was missed in the current changes.
It's just a wait for the replication, and should be reasonable to keep it without checking the output buffer on every OnWrite.
> 2. 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?
Yes, the lazy behavior is intentional behavior.
> 4. 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.
I prefer to keep rejecting the config explicitly instead of silence-and-ignore in Redis.
And I think we don't need to fix everything that pops up from AI reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-5011534737
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) **Quality Gate failed**
Failed conditions
 [40.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3560&metric=new_coverage&view=list) (required ≥ 50%)
 [E Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) (required ≥ A)
<!-- sqra:qg_revision=a7c89792975209931cf7bd006d158df2297d280f49774c214ff85621d1bc70b4 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560)
##
💡 **Need a hand with PR review?** Try [Gitar by Sonar](https://gitar.ai/?utm_source=sonarqube-cloud&utm_medium=in-product&utm_campaign=gitar-crosssell-q3-2026&utm_content=pr-deco)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-5010334276
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3560&metric=new_coverage&view=list) (required ≥ 50%)
 [B Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) (required ≥ A)
<!-- sqra:qg_revision=6ea18e088a77a18c32067a65e920265a5cc1338f84dbf8a4039b168b71a73bb5 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560)
##
💡 **Need a hand with PR review?** Try [Gitar by Sonar](https://gitar.ai/?utm_source=sonarqube-cloud&utm_medium=in-product&utm_campaign=gitar-crosssell-q3-2026&utm_content=pr-deco)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-5009775605
> 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.
Yes, I know this issue
> 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.
@jihuayu Yes, good catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(server): support the client-output-buffer-limit configuration [kvrocks]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3560:
URL: https://github.com/apache/kvrocks/pull/3560#issuecomment-4969693768
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) **Quality Gate failed**
Failed conditions
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=3560&metric=new_coverage&view=list) (required ≥ 50%)
 [B Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560) (required ≥ A)
<!-- sqra:qg_revision=6ea18e088a77a18c32067a65e920265a5cc1338f84dbf8a4039b168b71a73bb5 -->
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=3560)
##
💡 **Need a hand with PR review?** Try [Gitar by Sonar](https://gitar.ai/?utm_source=sonarqube-cloud&utm_medium=in-product&utm_campaign=gitar-crosssell-q3-2026&utm_content=pr-deco)!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org