Adding Missing filters to CLIENT LIST and Dedup Parsing by sarthakaggarwal97 · Pull Request #1401 · valkey-io/valkey

sarthakaggarwal97 · 2024-12-06T16:25:59Z

Adds filter options to CLIENT LIST:

* USER <username>
  Return clients authenticated by <username>.
* ADDR <ip:port>
  Return clients connected from the specified address.
* LADDR <ip:port>
  Return clients connected to the specified local address.
* SKIPME (YES|NO)
  Exclude the current client from the list (default: no).
* MAXAGE <maxage>
  Only list connections older than the specified age.

Modifies the ID filter to CLIENT KILL to allow multiple IDs

* ID <client-id> [<client-id>...]
  Kill connections by client ids.

This makes CLIENT LIST and CLIENT KILL accept the same options.

For backward compatibility, the default value for SKIPME is NO for CLIENT LIST and YES for CLIENT KILL.

The MAXAGE comes from CLIENT KILL, where it keeps clients with the given max age and kills the older ones. This logic becomes weird for CLIENT LIST, but is kept for similary with CLIENT KILL, for the use case of first testing manually using CLIENT LIST, and then running CLIENT KILL with the same filters.

The ID client-id [client-id ...] no longer needs to be the last filter. The parsing logic determines if an argument is an ID or not based on whether it can be parsed as an integer or not.

Partly addresses: #668

codecov · 2024-12-06T16:41:33Z

Codecov Report

Attention: Patch coverage is 91.83673% with 28 lines in your changes missing coverage. Please review.

Project coverage is 70.93%. Comparing base (d13aad4) to head (110c7d3).
Report is 5 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/networking.c	91.83%	28 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1401      +/-   ##
============================================
- Coverage     70.94%   70.93%   -0.01%     
============================================
  Files           120      120              
  Lines         65044    65078      +34     
============================================
+ Hits          46143    46161      +18     
- Misses        18901    18917      +16

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/server.h	`100.00% <ø> (ø)`
src/networking.c	`88.76% <91.83%> (+0.28%)`	⬆️

... and 14 files with indirect coverage changes

zuiderkwast

Thanks.

I just took a quick look and I can see some things that appear to be broken:

CLIENT LIST with multiple IDs, for example CLIENT LIST ID 3 4 5. (It means id is 3 or 4 or 5.) We a test case for this.
The default for "skipme" for CLIENT KILL should be YES by default (when filters are used), but for CLIENT LIST, skipme should be NO by default. Otherwise, we have a breaking change. Please add some tests without SKIPME but with other filters matching the calling client to cover the default skipme value for LIST and KILL.

We need some test cases combining multiple filter rules at the same time.

src/server.h

src/networking.c

sarthakaggarwal97 · 2024-12-09T17:29:41Z

Thanks @zuiderkwast for the comments! I am working on them

src/networking.c

sarthakaggarwal97 · 2024-12-11T09:28:12Z

@zuiderkwast thank you for your comments. Adressed most of them. Working on adding tests meanwhile. Trying to see how can I have tests with multiple clients.

Sharing some responses from local testing

Client Kill Command

127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=11 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=53 tot-net-out=209291 tot-cmds=1
id=4 addr=127.0.0.1:63141 laddr=127.0.0.1:6379 fd=9 name= age=8 idle=8 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
id=5 addr=127.0.0.1:63142 laddr=127.0.0.1:6379 fd=10 name= age=5 idle=5 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client info
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=16 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|info user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=79 tot-net-out=210285 tot-cmds=2
127.0.0.1:6379> client kill id 4
(integer) 1
127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=39 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=146 tot-net-out=210626 tot-cmds=4
id=5 addr=127.0.0.1:63142 laddr=127.0.0.1:6379 fd=10 name= age=33 idle=33 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=50 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=172 tot-net-out=211295 tot-cmds=5
id=5 addr=127.0.0.1:63142 laddr=127.0.0.1:6379 fd=10 name= age=44 idle=44 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
id=7 addr=127.0.0.1:63168 laddr=127.0.0.1:6379 fd=9 name= age=3 idle=3 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client kill id 5 7 
(integer) 2
127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=72 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=246 tot-net-out=212296 tot-cmds=7
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=7 idle=7 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
id=9 addr=127.0.0.1:63184 laddr=127.0.0.1:6379 fd=10 name= age=4 idle=4 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client kill id 3 9 addr 127.0.0.1:63183
(integer) 0
127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=97 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=352 tot-net-out=213295 tot-cmds=9
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=32 idle=32 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
id=9 addr=127.0.0.1:63184 laddr=127.0.0.1:6379 fd=10 name= age=29 idle=29 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client kill id 3 9 addr 127.0.0.1:63184
(integer) 1
127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=107 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=458 tot-net-out=214298 tot-cmds=11
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=42 idle=42 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1

Client List Command

127.0.0.1:6379> client list
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=1361 idle=8 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=484 tot-net-out=215973 tot-cmds=13
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=1296 idle=1296 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
id=10 addr=127.0.0.1:55765 laddr=127.0.0.1:6379 fd=10 name= age=10 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=10 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1946 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=53 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client list id 3 8
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=1368 idle=15 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=484 tot-net-out=215973 tot-cmds=13
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=1303 idle=1303 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client list id 3 8 age 1300
ERR syntax error
127.0.0.1:6379> client list id 3 8 maxage 1300
id=3 addr=127.0.0.1:63139 laddr=127.0.0.1:6379 fd=8 name= age=1387 idle=34 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=484 tot-net-out=215973 tot-cmds=13
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=1322 idle=1322 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1
127.0.0.1:6379> client list id 3 8 maxage 1500
127.0.0.1:6379> client list id 3 8 addr 127.0.0.1:63183
id=8 addr=127.0.0.1:63183 laddr=127.0.0.1:6379 fd=9 name= age=1349 idle=1349 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=0 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=1920 events=r cmd=command|docs user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=27 tot-net-out=209291 tot-cmds=1

zuiderkwast

Trying to see how can I have tests with multiple clients.

You can create multiple clients.

        set c1 [valkey_client]
        set c2 [valkey_client]
        set c3 [valkey_client]
        ...
        $c1 close
        $c2 close
        $c3 close

src/networking.c

zuiderkwast

The refactoring looks good. I have just a few small comments and there is some merge conflict.

src/networking.c

zuiderkwast

LGTM. Just two nits.

src/networking.c

zuiderkwast · 2024-12-20T18:10:07Z

@valkey-io/core-team Please approve the extended syntax for CLIENT LIST as described in the top comment.

The MAXAGE argument for CLIENT LIST is a little strange, because it's the max age of clients to exclude, for symmetry with CLIENT KILL. An alternative is to forbid MAXAGE for CLIENT LIST.

src/networking.c

sarthakaggarwal97 · 2025-01-14T23:12:17Z

Thanks folks for the suggestions and inputs.

Based on @hwware suggestions, I performed the SET/GET operations while running client list <filter> <filter-value> for 10000 clients.
I do not see any major deviation in performance from the current unstable command.

Sharing the results:

Notes:

Latency Unit: msec
RPS: requests per second

hpatro · 2025-01-14T23:19:06Z

@sarthakaggarwal97 Thanks for sharing the results and is inline with our expectation. I'm good with the stats. The above benchmarking is performed with one client running the CLIENT LIST (filters) and 10k clients running SET/GET operations. The setup is quite similar to a real world scenario where the no. of application clients >>> no. of administrator clients.

hwware · 2025-01-15T16:09:26Z

It looks good to me. Approved

hpatro · 2025-01-15T18:33:11Z

I see atleast 4 core-team member have approved this either through voting or PR approval. @valkey-io/core-team Do you want to review further or is it good to be merged?

zuiderkwast · 2025-01-15T19:43:17Z

Let's merge it.

@sarthakaggarwal97 do you want to update the docs too? The commands/*.md files in the valkey-doc repo. We can have one doc PR for all these updates if you want.

sarthakaggarwal97 · 2025-01-15T20:00:03Z

Thank you everyone for helping in this one.

@zuiderkwast let me raise the doc PR as well. Thank you!

Adds filter options to CLIENT LIST: * USER <username> Return clients authenticated by <username>. * ADDR <ip:port> Return clients connected from the specified address. * LADDR <ip:port> Return clients connected to the specified local address. * SKIPME (YES|NO) Exclude the current client from the list (default: no). * MAXAGE <maxage> Only list connections older than the specified age. Modifies the ID filter to CLIENT KILL to allow multiple IDs * ID <client-id> [<client-id>...] Kill connections by client ids. This makes CLIENT LIST and CLIENT KILL accept the same options. For backward compatibility, the default value for SKIPME is NO for CLIENT LIST and YES for CLIENT KILL. The MAXAGE comes from CLIENT KILL, where it *keeps* clients with the given max age and kills the older ones. This logic becomes weird for CLIENT LIST, but is kept for similary with CLIENT KILL, for the use case of first testing manually using CLIENT LIST, and then running CLIENT KILL with the same filters. The `ID client-id [client-id ...]` no longer needs to be the last filter. The parsing logic determines if an argument is an ID or not based on whether it can be parsed as an integer or not. Partly addresses: valkey-io#668 --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com> Signed-off-by: proost <jwalag87@gmail.com>

Documentation for valkey-io/valkey#1401 --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

Adds filter options to CLIENT LIST: * USER <username> Return clients authenticated by <username>. * ADDR <ip:port> Return clients connected from the specified address. * LADDR <ip:port> Return clients connected to the specified local address. * SKIPME (YES|NO) Exclude the current client from the list (default: no). * MAXAGE <maxage> Only list connections older than the specified age. Modifies the ID filter to CLIENT KILL to allow multiple IDs * ID <client-id> [<client-id>...] Kill connections by client ids. This makes CLIENT LIST and CLIENT KILL accept the same options. For backward compatibility, the default value for SKIPME is NO for CLIENT LIST and YES for CLIENT KILL. The MAXAGE comes from CLIENT KILL, where it *keeps* clients with the given max age and kills the older ones. This logic becomes weird for CLIENT LIST, but is kept for similary with CLIENT KILL, for the use case of first testing manually using CLIENT LIST, and then running CLIENT KILL with the same filters. The `ID client-id [client-id ...]` no longer needs to be the last filter. The parsing logic determines if an argument is an ID or not based on whether it can be parsed as an integer or not. Partly addresses: valkey-io#668 --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

@zuiderkwast

small change to [#1401](#1401). pass `clientFilter *` to clientMatchesFilter as @zuiderkwast suggest in [comment](6bc64ca#r1875844273). Signed-off-by: Ray Cao <zisong.cw@alibaba-inc.com>

small change to [#1401](#1401). pass `clientFilter *` to clientMatchesFilter as suggest in [comment](6bc64ca#r1875844273). Signed-off-by: Ray Cao <zisong.cw@alibaba-inc.com>

small change to [valkey-io#1401](valkey-io#1401). pass `clientFilter *` to clientMatchesFilter as suggest in [comment](valkey-io@6bc64ca#r1875844273). Signed-off-by: Ray Cao <zisong.cw@alibaba-inc.com>

With #1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) #1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 #2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 #3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 #4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 #5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 #6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 #7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 #8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 #9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 #10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 #11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 #12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 #13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 #14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 #15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 #16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>

With valkey-io#1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) #1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 #2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 #3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 valkey-io#16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>

With valkey-io#1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 valkey-io#16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com> (cherry picked from commit 155b0bb)

With valkey-io#1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 valkey-io#16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com> (cherry picked from commit 155b0bb) Signed-off-by: cherukum-amazon <cherukum@amazon.com>

With #1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) #1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 #2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 #3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 #4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 #5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 #6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 #7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 #8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 #9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 #10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 #11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 #12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 #13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 #14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 #15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 #16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com> (cherry picked from commit 155b0bb) Signed-off-by: cherukum-amazon <cherukum@amazon.com>

sarthakaggarwal97 force-pushed the client-command-revamp branch from 54adfa6 to 7630eba Compare December 9, 2024 00:58

sarthakaggarwal97 marked this pull request as ready for review December 9, 2024 00:59

sarthakaggarwal97 requested review from hpatro and zuiderkwast December 9, 2024 00:59

zuiderkwast reviewed Dec 9, 2024

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

sarthakaggarwal97 mentioned this pull request Dec 10, 2024

Extended Valkey client introspection functionality #668

Open

3 tasks

zuiderkwast reviewed Dec 10, 2024

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

sarthakaggarwal97 force-pushed the client-command-revamp branch 2 times, most recently from 049f2d4 to e3c1c5a Compare December 11, 2024 09:23

zuiderkwast reviewed Dec 11, 2024

View reviewed changes

sarthakaggarwal97 force-pushed the client-command-revamp branch 3 times, most recently from fd743fe to 6681e47 Compare December 16, 2024 21:00

zuiderkwast reviewed Dec 16, 2024

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

sarthakaggarwal97 force-pushed the client-command-revamp branch from 6681e47 to 52fd611 Compare December 16, 2024 22:41

sarthakaggarwal97 mentioned this pull request Dec 20, 2024

[Client Introspection] Client Commands Extended Filtering #1466

Merged

sarthakaggarwal97 force-pushed the client-command-revamp branch from 52fd611 to ac3f5e5 Compare December 20, 2024 17:16

zuiderkwast reviewed Dec 20, 2024

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

src/networking.c Outdated Show resolved Hide resolved

zuiderkwast added release-notes This issue should get a line item in the release notes major-decision-pending Major decision pending by TSC team needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. labels Dec 20, 2024

zuiderkwast reviewed Dec 20, 2024

View reviewed changes

src/networking.c Outdated Show resolved Hide resolved

hpatro reviewed Dec 20, 2024

View reviewed changes

sarthakaggarwal97 force-pushed the client-command-revamp branch from 3b789c2 to a944e0e Compare December 20, 2024 19:26

sarthakaggarwal97 mentioned this pull request Dec 20, 2024

[Client Introspection] Client Count Command #1467

Closed

sarthakaggarwal97 force-pushed the client-command-revamp branch from 2120e61 to 301993f Compare December 20, 2024 23:43

hwware approved these changes Jan 15, 2025

View reviewed changes

zuiderkwast added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Jan 15, 2025

zuiderkwast merged commit 6a8f068 into valkey-io:unstable Jan 15, 2025
51 checks passed

sarthakaggarwal97 mentioned this pull request Jan 16, 2025

Updated Documentation for CLIENT KILL and CLIENT LIST valkey-io/valkey-doc#218

Merged

zuiderkwast pushed a commit to valkey-io/valkey-doc that referenced this pull request Jan 17, 2025

Updated Documentation for CLIENT KILL and CLIENT LIST (#218)

af26ec2

Documentation for valkey-io/valkey#1401 --------- Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>

RayaCoo mentioned this pull request Feb 14, 2025

Modify parameter of clientMatchesFilter function. #1733

Merged

ranshid mentioned this pull request Mar 25, 2025

Valkey 8.1.0-ga release checklist #1881

Closed

12 tasks

hpatro mentioned this pull request Jul 16, 2025

Fix memory leak with CLIENT LIST/KILL duplicate filters #2362

Merged

Comments

Conversation

sarthakaggarwal97 commented Dec 6, 2024 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarthakaggarwal97 commented Dec 9, 2024

Uh oh!

Uh oh!

Uh oh!

sarthakaggarwal97 commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Dec 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarthakaggarwal97 commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hpatro commented Jan 14, 2025

Uh oh!

hwware commented Jan 15, 2025

Uh oh!

hpatro commented Jan 15, 2025

Uh oh!

zuiderkwast commented Jan 15, 2025

Uh oh!

Uh oh!

sarthakaggarwal97 commented Jan 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sarthakaggarwal97 commented Dec 6, 2024 •

edited by zuiderkwast

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

sarthakaggarwal97 commented Dec 11, 2024 •

edited

Loading

zuiderkwast left a comment •

edited

Loading

sarthakaggarwal97 commented Jan 14, 2025 •

edited

Loading