Skip to content
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

AbstractConnection.register_connect_callback was made private in version 5.0.1 #2965

Closed
woutdenolf opened this issue Sep 27, 2023 · 14 comments
Closed

Comments

@woutdenolf
Copy link
Contributor

woutdenolf commented Sep 27, 2023

Version: redis-py 5.0.1

Platform: Ubuntu 20.04

Description:

In #2870 the public API of AbstractConnection changed

# redis-py 5.0.0

class AbstractConnection:
    def register_connect_callback(self, callback):
# redis-py 5.0.1

class AbstractConnection:
    def _register_connect_callback(self, callback):

We use the public method register_connect_callback in our code:

  File "/home/denolf/dev/bliss/blissdata/blissdata/redis/caching.py", line 150, in _create_invalidation_connection
    connection.register_connect_callback(stop_callback)
AttributeError: 'UnixDomainSocketConnection' object has no attribute 'register_connect_callback'
@woutdenolf
Copy link
Contributor Author

Ping @kristjanvalur

@kristjanvalur
Copy link
Contributor

kristjanvalur commented Sep 27, 2023

Ah, this was done as a result to this comment here: #2870 (comment)
by @chayim. These apis were supposed to be internal, I guess, since they were designed for the PubSub class, I gather. There previously was no "unregister" (only clear) and so it wasn't really well suited for non-library code.
I'm happy to change it back, to remove the underscore, if that is what the maintainers decide.

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Sep 28, 2023

Same goes for clear_connect_callbacks: it was replace by _deregister_connect_callback.

@kristjanvalur
Copy link
Contributor

kristjanvalur commented Sep 28, 2023

Yes, that one was problematic because it clears them all, not just the one you registered. But it is not really needed because the callbacks are weak anyway.

@hiimdoublej-swag
Copy link

Hello @kristjanvalur,
We have a use case that is currently achieved by registering our own callbacks using register_connect_callback
Are there plans to bring the public functions back ?

@kristjanvalur
Copy link
Contributor

Well, I'm not a maintainer here, just a submitter of pull-requests. I can prepare one where we move these functions back, but it is up to the maintainers whether they want them. I think it would help your argument if you could explain your use case, because as I understand, these apis were only ever intended to be private to begin with.

@hiimdoublej-swag
Copy link

hiimdoublej-swag commented Oct 5, 2023

@chayim Need your opinion on restoring these functions, please kindly take a look at this thread, thanks.

@woutdenolf
Copy link
Contributor Author

woutdenolf commented Oct 5, 2023

We use it for client side caching in which you need to set TRACKING on upon connecting (off when disconnecting) to invalidate client side cache when a value changes on the server.

@kristjanvalur
Copy link
Contributor

We use it for client side caching in which you need to set TRACKING on upon connecting (off when disconnecting) to invalidate client side cache when a value changes on the server.

cool use case. Yes, I totally get it, the client needs to know when the connection was automatically re-connected in order to enable tracking again. Who knows what other ephemeral connection state a client might want to set on a re-connected connection?

I'll prepare a PR making those non-semi-private again, lets see what the maintainers think. I can't be bothered to document those though :)

@hiimdoublej-swag
Copy link

ping @chayim

@kristjanvalur
Copy link
Contributor

in the mean-time, these aren't private-private, you can continue to use them with the leading underscore. It is called hacking and all the cool kids are doing it, I'm told.

@syphar
Copy link

syphar commented Oct 19, 2023

Just to add here, register_connect_callback is used by the celery redis result backend:
https://github.com/celery/celery/blob/0bc89cc594638e1d88655764807fcc59fb32efc6/celery/backends/redis.py#L122

So with redis-py 5.0.1 we see errors with checking celery results in some cases

I can dig deeper if more information is needed, will skip 5.0.1 as a quickfix

@Shineaki
Copy link

Shineaki commented Dec 8, 2023

Thanks for this post, I just ran into this bug and wasn't sure what to make of it:

Exception ignored in: <function AsyncResult.__del__ at 0x7f7222a3b9d0> Traceback (most recent call last): File "/usr/local/lib/python3.9/site-packages/celery/result.py", line 417, in __del__ self.backend.remove_pending_result(self) File "/usr/local/lib/python3.9/site-packages/celery/backends/asynchronous.py", line 208, in remove_pending_result self.on_result_fulfilled(result) File "/usr/local/lib/python3.9/site-packages/celery/backends/asynchronous.py", line 216, in on_result_fulfilled self.result_consumer.cancel_for(result.id) File "/usr/local/lib/python3.9/site-packages/celery/backends/redis.py", line 184, in cancel_for self._pubsub.unsubscribe(key) File "/usr/local/lib/python3.9/contextlib.py", line 137, in __exit__ self.gen.throw(typ, value, traceback) File "/usr/local/lib/python3.9/site-packages/celery/backends/redis.py", line 130, in reconnect_on_error self._ensure(self._reconnect_pubsub, ()) File "/usr/local/lib/python3.9/site-packages/celery/backends/redis.py", line 384, in ensure return retry_over_time( File "/usr/local/lib/python3.9/site-packages/kombu/utils/functional.py", line 318, in retry_over_time return fun(*args, **kwargs) File "/usr/local/lib/python3.9/site-packages/celery/backends/redis.py", line 122, in _reconnect_pubsub self._pubsub.connection.register_connect_callback(self._pubsub.on_connect) AttributeError: 'SSLConnection' object has no attribute 'register_connect_callback'

  • Redis = 5.0.1
  • Celery = 5.3.6

@dvora-h
Copy link
Collaborator

dvora-h commented Feb 11, 2024

Fixed in #2980

@dvora-h dvora-h closed this as completed Feb 11, 2024
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

No branches or pull requests

6 participants