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

Sync cached_property getter access #150

Merged

Conversation

mjpieters
Copy link
Contributor

@mjpieters mjpieters commented Jul 23, 2024

Add an option to pass in a lock to the cached_property decorator, so the
accessor is run just once even when multiple tasks are concurrently trying to
await on the value.

This PR fixes concurrent access to the property getter. Notable changes include:

  • The cached_property decorator now accepts either a getter coroutine method,
    or an async context manager. In the latter case a new decorator is returned
    to accept the actual getter.
  • Aligned cached_property internals with the stdlib.functools.cached_property version. Most notably, the test for instance.__dict__ has been moved to __get__, and the __wrapped__ and _name attributes now match the attribtues on a stdlib cached property descriptor.
  • Improved the type annotations; e.g. the return type of the awaitable getter is covariant.

Closes #149

@mjpieters mjpieters force-pushed the sync_cached_property_getter branch from fa39a6b to 7bac360 Compare July 23, 2024 17:48
asyncstdlib/functools.py Fixed Show fixed Hide fixed
asyncstdlib/functools.py Fixed Show fixed Hide fixed
asyncstdlib/functools.py Fixed Show fixed Hide fixed
asyncstdlib/functools.py Fixed Show fixed Hide fixed
@maxfischer2781
Copy link
Owner

maxfischer2781 commented Jul 23, 2024

The CodeQL "Statement has no effect" reports for ellipsis all are false positives and can be ignored: github/codeql#11351

@maxfischer2781
Copy link
Owner

Thanks for this nice work! I need some time to review it, but at a glance I don't see any glaring problems.

@maxfischer2781 maxfischer2781 self-requested a review July 23, 2024 19:07
Copy link
Owner

@maxfischer2781 maxfischer2781 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 changes. That is some nicely thought out code, I had to go back a few times when realising I misunderstood an edge case that you considered.

Aside from some minor things, there's three main points that I stumbled on:

  • I don't think the fast path in _await_impl is worth the code size cost. Please correct me if I'm missing a reason to keep it.
  • The locked section in _await_impl could drop the lock more aggressively.
  • I'm not happy with either variant of the new parameter name. Unless you have an idea for a better one, I prefer to not expose it by making the parameter positional-only.

asyncstdlib/functools.py Outdated Show resolved Hide resolved
asyncstdlib/functools.py Outdated Show resolved Hide resolved
asyncstdlib/functools.py Outdated Show resolved Hide resolved
asyncstdlib/functools.py Outdated Show resolved Hide resolved
asyncstdlib/functools.py Outdated Show resolved Hide resolved
asyncstdlib/functools.pyi Outdated Show resolved Hide resolved
docs/source/api/functools.rst Outdated Show resolved Hide resolved
@mjpieters mjpieters force-pushed the sync_cached_property_getter branch 2 times, most recently from f60d000 to 73b981b Compare August 1, 2024 15:30
@mjpieters
Copy link
Contributor Author

This should be ready for re-review now.

Add an option to pass in a lock to the cached_property decorator,
so the accessor is run just once even when multiple tasks are
concurrently trying to await on the value.
@mjpieters mjpieters force-pushed the sync_cached_property_getter branch from 73b981b to d2e442f Compare August 1, 2024 15:39
@maxfischer2781
Copy link
Owner

The static check failures are unrelated and will be fixed by #152, no need to duplicate them here.

@maxfischer2781 maxfischer2781 merged commit e08a044 into maxfischer2781:master Aug 4, 2024
10 of 11 checks passed
@maxfischer2781 maxfischer2781 mentioned this pull request Aug 5, 2024
@mjpieters mjpieters deleted the sync_cached_property_getter branch August 20, 2024 13:32
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 this pull request may close these issues.

[Feature]: @cached_property that only awaits the wrapped method _once_.
2 participants