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

Kombu may rely on locking in functools.cached_property that will be removed in Python 3.12 #1668

Open
Tracked by #211
carljm opened this issue Mar 14, 2023 · 9 comments

Comments

@carljm
Copy link

carljm commented Mar 14, 2023

Hello! This is just a heads-up that the lock in functools.cached_property was deemed to be a bug (see python/cpython#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.

@open-collective-bot
Copy link

Hey @carljm 👋,
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.

@auvipy
Copy link
Member

auvipy commented Mar 14, 2023

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

@jrobichaud
Copy link

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:
python/cpython@056dfc7#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

@nickjj
Copy link

nickjj commented Oct 3, 2023

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'

@auvipy
Copy link
Member

auvipy commented Oct 3, 2023

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

@MythicManiac
Copy link

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.

@nickjj
Copy link

nickjj commented Dec 9, 2023

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: nickjj/docker-flask-example@21189ac

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.

@MythicManiac
Copy link

@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?

@auvipy
Copy link
Member

auvipy commented Dec 10, 2023

I thought it was fixed by 3ad075a ? but may be this one is different, right?

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

5 participants