kombu

kombu copied to clipboard

Hello! This is just a heads-up that the lock in functools.cached_property was deemed to be a bug (see https://github.com/python/cpython/issues/87634) because it is class-wide, meaning that it can lead to pathological lock contention if there are lots of instances. The chosen resolution for functools.cached_property was to remove the locking behavior entirely in Python 3.12. This means that in 3.12, it will be possible for a cached_property getter func to run multiple times for the same instance, if used in a multi-threaded context.

I'm filing this issue because I see that in https://github.com/celery/kombu/blob/main/kombu/utils/objects.py you use functools.cached_property if available, with a fallback to threaded_cached_property from the cached_property third-party library.

If you need the locking, and you aren't affected by high lock contention due to the class-wide nature of the lock, you may want to simply unconditionally rely on cached_property.threaded_cached_property going forward, and not use functools.cached_property. Alternately, if you want to use functools.cached_property, you can replicate the current behavior by putting a threading.RLock on the class and checking it inside your cached property getter function.

Hey @carljm :wave:, Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider backing us - every little helps!

We also offer priority support for our sponsors. If you require immediate assistance please consider sponsoring us.

thanks for the heads up Carl! we will certainly consider your suggestions and also investigate further into it.

I'd also like to mention _NOT_FOUND has been removed from functools in python 3.12 which force to always fallback to the module cached_property. (ironic isn't?)

The change: https://github.com/python/cpython/commit/056dfc71dce15f81887f0bd6da09d6099d71f979#diff-8657d1a12c81ea9da8c75d3de3827fc6f1e033339453e9587f1ec2f5dfc5111dL962

kombu's code:

try:
    from functools import _NOT_FOUND
    from functools import cached_property as _cached_property
except ImportError:
    # TODO: Remove this fallback once we drop support for Python < 3.8
    from cached_property import threaded_cached_property as _cached_property

    _NOT_FOUND = object()

https://github.com/celery/kombu/blob/7516daf/kombu/utils/objects.py#L7-L14

Now that Python 3.12 is officially out, yep I did run into this when switching a project over to it that uses Celery:

  File "/app/hello/app.py", line 94, in <module>
    celery_app = create_celery_app()
                 ^^^^^^^^^^^^^^^^^^^
  File "/app/hello/app.py", line 34, in create_celery_app
    celery.Task = ContextTask
    ^^^^^^^^^^^
  File "/home/python/.local/lib/python3.12/site-packages/kombu/utils/objects.py", line 37, in __set__
    with self.lock:
         ^^^^^^^^^
AttributeError: 'cached_property' object has no attribute 'lock'

will look into this soon. as python 3.12 just got released, it will take little bit longer to fully support it. But will try to do ASAP. if all the dependencies work with python 3.12

Just ran into this as well. I'd contribute a fix but I'm not familiar enough with the kombu codebase to know why a lock was used here in the first place, so I don't think I'm the appropriate person for the job.

If anyone is curious, the current version of Kombu does work with Python 3.12 but you might need to adjust how you've initialized Celery in the past.

For example with Flask it's common to have a create_celery_app function. Here's a diff that's compatible with Python 3.12: https://github.com/nickjj/docker-flask-example/commit/21189acea4983015a5b79bc8fa57fda14233d3c6

It also works with Django too and nothing different had to be done for Python 3.12, here's a repo pulling it all together: https://github.com/nickjj/docker-django-example

Both of the above examples are using the latest version of their respective frameworks which is Flask 3.0.X and Django 5.0.X.

IMO this issue should still be addressed since Kombu's internal code base is using a feature that no longer works with Python 3.12 but for now there is a path forward to use Python 3.12 for a number of Celery use cases.

@nickjj thanks for the workaround, from what I gather it changes the base task class in a way that doesn't end up calling the problematic code path? Any idea how comprehensive that workaround is e.g. in regards to other celery APIs that could lead to it being triggered?

I thought it was fixed by https://github.com/celery/kombu/commit/3ad075a536d177170a7a5d7b04d81e94bd5bf404 ? but may be this one is different, right?