Skip to content

Client name doesn't persist after lost connection when set using client_setname #2245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
psrok1 opened this issue Jun 22, 2022 · 3 comments · Fixed by #2247
Closed

Client name doesn't persist after lost connection when set using client_setname #2245

psrok1 opened this issue Jun 22, 2022 · 3 comments · Fixed by #2247

Comments

@psrok1
Copy link
Contributor

psrok1 commented Jun 22, 2022

Version: redis-py 4.3.3

Platform: Python 3.10 / Ubuntu 22.04

Description:

Hello there! When client name is set using client_setname, it doesn't persist after reconnection.

In [3]: r = redis.Redis.from_url("redis://redis/")
In [4]: r.client_setname("test-name")
Out[4]: True
In [5]: r.client_getname()
Out[5]: 'test-name'
In [6]: r.connection_pool.disconnect()
In [7]: r.client_getname()

I noticed that client name persists only if it's passed to the Redis object constructor, so it's set in ConnectionPool and correctly used for new connections:

In [8]: r = redis.Redis.from_url("redis://redis/", client_name="test-name")
In [9]: r.client_getname()
Out[9]: 'test-name'
In [10]: r.connection_pool.disconnect()
In [11]: r.client_getname()
Out[11]: 'test-name'

Is it intended behavior? If yes, maybe there should be a warning about it in documentation, because it may lead to unexpected results e.g. during debugging of connection issues.

@chayim
Copy link
Contributor

chayim commented Jun 22, 2022

Hi! This is actually the intended behaviour. We link to the documentation for CLIENT SETNAME to try and help!

So, CLIENT SETNAME allows one to attach names to a client, so that using things like CLIENT LIST makes the possibility to manage clients easier (say looping through a list and issuing a CLIENT KILL). This name only exists to the currently connected client.

If there's a documentation update that you think could help users, that would be awesome, and we could definitely look at merging it. WDYT?

@psrok1
Copy link
Contributor Author

psrok1 commented Jun 22, 2022

Ok, I see this is by design - commands are executed only in the context of current connection, so common client name should be set on different layer. Actually it would be difficult to have common behavior e.g. in case of multiprocessing.

I would go with a little note in https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.core.CoreCommands.client_setname: "This method sets client name only for current connection. If you want to set a common name for all connections managed by this client, see client_name argument". I think it might be helpful because Client provides an abstraction that behaves like a single connection, hiding the connection management underneath.

If you agree on that, I will come back with a PR then!

@dvora-h
Copy link
Collaborator

dvora-h commented Jun 23, 2022

@psrok1 Sounds great, waiting for a PR if you will. (LMK if not and I will do it myself)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants