Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement a CachedCall to handle boilerplate of caching results #9353

Closed
wants to merge 2 commits into from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Feb 8, 2021

This feels like a relatively common pattern which is hard to get right.

I've used oidc.jwks as an example for how it could be used.

This feels like a relatively common pattern which is hard to get right.

I've used `oidc.jwks` as an example for how it could be used.
@richvdh richvdh requested a review from a team February 8, 2021 20:02
@richvdh
Copy link
Member Author

richvdh commented Feb 8, 2021

I'm interested in thoughts on this. I don't think the tests are passing; in any case, if it's a good idea, I want to add unit tests. But if the answer is "why don't you just ....", then I won't pursue it further.

self._deferred = None # type: Optional[Deferred]
self._result = None # type: Union[None, Failure, TV]

async def get(self) -> TV:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe slighter nicer ergonomics if we used __call__ instead?

This would let you do something like:

async def handle_request() -> X:
    # We can call this multiple times, but it will result in a single call to
    # _load_prop().
    return await _cached_val()

This feels a bit nicer since you've wrapped a function and then it returns a function-like thing and might let you use it as a decorator?

Copy link
Member Author

Choose a reason for hiding this comment

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

hrm, maybe. I always feel like relying on __call__ is a bit magical, and tend to prefer that the interactions are made explicit, which is why I did it this way. I could be persuaded though.

Copy link
Member

Choose a reason for hiding this comment

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

My thought is that it makes it more like functools.cached_property, but perhaps that isn't the goal. 😄

Comment on lines +93 to +97
# I *think* this is the easiest way to correctly raise a Failure without having
# to gut-wrench into the implementation of Deferred.
d = Deferred()
d.callback(self._result)
return await d
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this will work properly for errors, do we need to call d.errback if it is an instance of Failure? (I don't think Deferred does anything special if you callback a Failure.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work, because errback(x) and callback(x) are both just thin wrappers around _startRunCallbacks(x). You could argue that this isn't using Deferred's API as it's intended, which might be fair.

@richvdh
Copy link
Member Author

richvdh commented Feb 9, 2021

This is now superceded by #9362.

@richvdh richvdh closed this Feb 9, 2021
@richvdh richvdh deleted the rav/cached_call branch April 6, 2022 12:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants