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

Create a lock on cached_property if not present #1811

Merged
merged 2 commits into from Oct 18, 2023

Conversation

tari
Copy link
Contributor

@tari tari commented Oct 12, 2023

This fixes #1804 (fixing breakage caused by use of undocumented implementation details of functools.cached_property) by ensuring a lock is always present on cached_property attributes, which is required to safely support setting and deleting cached values in addition to computing them on demand.

@auvipy auvipy self-requested a review October 12, 2023 05:15
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the patch. do you think the change should be covered with some unit tests at least?

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are hit by a test failure with python 3.12 here #1812 (comment), was thinking if this change going to fix that? or we need to update the test as well?

@auvipy auvipy added this to the 5.3.x milestone Oct 12, 2023
@tari
Copy link
Contributor Author

tari commented Oct 12, 2023

thanks for the patch. do you think the change should be covered with some unit tests at least?

Yes, I should be able to add a test to verify that locks are held as needed.

We are hit by a test failure with python 3.12 here #1812 (comment), was thinking if this change going to fix that? or we need to update the test as well?

That looks like a different issue, though I didn't look at the test very closely.

@tari
Copy link
Contributor Author

tari commented Oct 12, 2023

thanks for the patch. do you think the change should be covered with some unit tests at least?

Added a test that verifies the lock is acquired for all of get, set, and delete operations. I could also test that the lock is held at appropriate times, but opted not to do that since it would require doing a threading dance in the test to verify; just ensuring that the operations acquire the lock seems fine.

tari and others added 2 commits October 12, 2023 23:15
This fixes celery#1804 (fixing breakage caused by use of undocumented
implementation details of functools.cached_property) by ensuring a lock
is always present on cached_property attributes, which is required to
safely support setting and deleting cached values in addition to
computing them on demand.
@auvipy auvipy merged commit 3ad075a into celery:main Oct 18, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Assigning to cached_property is broken on Python 3.12
3 participants