Unverified Commit e7f18432 authored by judeng's avatar judeng Committed by GitHub
Browse files

avoid incorrect shrinking of querybuf when client is reading a big argv (#12000)

this pr fix two wrongs:
1. When client’s querybuf is pre-allocated for a fat argv, we need to update the
  querybuf_peak of the client immediately to completely avoid the unexpected
  shrinking of querybuf in the next clientCron (before data arrives to set the peak).
2. the protocol's bulklen does not include `\r\n`, but the allocation and the data we
  read does. so in `clientsCronResizeQueryBuffer`, the `resize` or `querybuf_peak`
  should add these 2 bytes.

the first bug is likely to hit us on large payloads over slow connections, in which case
transferring the payload can take longer and a cron event will be triggered (specifically
if there are not a lot of clients)
parent 1222b608
...@@ -2342,6 +2342,9 @@ int processMultibulkBuffer(client *c) { ...@@ -2342,6 +2342,9 @@ int processMultibulkBuffer(client *c) {
/* Hint the sds library about the amount of bytes this string is /* Hint the sds library about the amount of bytes this string is
* going to contain. */ * going to contain. */
c->querybuf = sdsMakeRoomForNonGreedy(c->querybuf,ll+2-sdslen(c->querybuf)); c->querybuf = sdsMakeRoomForNonGreedy(c->querybuf,ll+2-sdslen(c->querybuf));
/* We later set the peak to the used portion of the buffer, but here we over
* allocated because we know what we need, make sure it'll not be shrunk before used. */
if (c->querybuf_peak < (size_t)ll + 2) c->querybuf_peak = ll + 2;
} }
} }
c->bulklen = ll; c->bulklen = ll;
...@@ -2636,6 +2639,9 @@ void readQueryFromClient(connection *conn) { ...@@ -2636,6 +2639,9 @@ void readQueryFromClient(connection *conn) {
* the query buffer, we also don't wanna use the greedy growth, in order * the query buffer, we also don't wanna use the greedy growth, in order
* to avoid collision with the RESIZE_THRESHOLD mechanism. */ * to avoid collision with the RESIZE_THRESHOLD mechanism. */
c->querybuf = sdsMakeRoomForNonGreedy(c->querybuf, readlen); c->querybuf = sdsMakeRoomForNonGreedy(c->querybuf, readlen);
/* We later set the peak to the used portion of the buffer, but here we over
* allocated because we know what we need, make sure it'll not be shrunk before used. */
if (c->querybuf_peak < qblen + readlen) c->querybuf_peak = qblen + readlen;
} else { } else {
c->querybuf = sdsMakeRoomFor(c->querybuf, readlen); c->querybuf = sdsMakeRoomFor(c->querybuf, readlen);
......
...@@ -744,7 +744,7 @@ int clientsCronResizeQueryBuffer(client *c) { ...@@ -744,7 +744,7 @@ int clientsCronResizeQueryBuffer(client *c) {
* sure not to resize to less than the bulk length. */ * sure not to resize to less than the bulk length. */
size_t resize = sdslen(c->querybuf); size_t resize = sdslen(c->querybuf);
if (resize < c->querybuf_peak) resize = c->querybuf_peak; if (resize < c->querybuf_peak) resize = c->querybuf_peak;
if (c->bulklen != -1 && resize < (size_t)c->bulklen) resize = c->bulklen; if (c->bulklen != -1 && resize < (size_t)c->bulklen + 2) resize = c->bulklen + 2;
c->querybuf = sdsResize(c->querybuf, resize, 1); c->querybuf = sdsResize(c->querybuf, resize, 1);
} }
} }
...@@ -754,8 +754,7 @@ int clientsCronResizeQueryBuffer(client *c) { ...@@ -754,8 +754,7 @@ int clientsCronResizeQueryBuffer(client *c) {
c->querybuf_peak = sdslen(c->querybuf); c->querybuf_peak = sdslen(c->querybuf);
/* We reset to either the current used, or currently processed bulk size, /* We reset to either the current used, or currently processed bulk size,
* which ever is bigger. */ * which ever is bigger. */
if (c->bulklen != -1 && (size_t)c->bulklen > c->querybuf_peak) if (c->bulklen != -1 && (size_t)c->bulklen + 2 > c->querybuf_peak) c->querybuf_peak = c->bulklen + 2;
c->querybuf_peak = c->bulklen;
return 0; return 0;
} }
......
...@@ -18,6 +18,8 @@ proc client_query_buffer {name} { ...@@ -18,6 +18,8 @@ proc client_query_buffer {name} {
} }
start_server {tags {"querybuf slow"}} { start_server {tags {"querybuf slow"}} {
# increase the execution frequency of clientsCron
r config set hz 100
# The test will run at least 2s to check if client query # The test will run at least 2s to check if client query
# buffer will be resized when client idle 2s. # buffer will be resized when client idle 2s.
test "query buffer resized correctly" { test "query buffer resized correctly" {
...@@ -63,4 +65,26 @@ start_server {tags {"querybuf slow"}} { ...@@ -63,4 +65,26 @@ start_server {tags {"querybuf slow"}} {
assert {[client_query_buffer test_client] > 0 && [client_query_buffer test_client] < $orig_test_client_qbuf} assert {[client_query_buffer test_client] > 0 && [client_query_buffer test_client] < $orig_test_client_qbuf}
$rd close $rd close
} }
test "query buffer resized correctly with fat argv" {
set rd [redis_client]
$rd client setname test_client
$rd write "*3\r\n\$3\r\nset\r\n\$1\r\na\r\n\$1000000\r\n"
$rd flush
after 20
if {[client_query_buffer test_client] < 1000000} {
fail "query buffer should not be resized when client idle time smaller than 2s"
}
# Check that the query buffer is resized after 2 sec
wait_for_condition 1000 10 {
[client_idle_sec test_client] >= 3 && [client_query_buffer test_client] < 1000000
} else {
fail "query buffer should be resized when client idle time bigger than 2s"
}
$rd close
}
} }
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment