Implement initial lazy caching credentials provider #578
Conversation
There was a problem hiding this comment.
My feeling here is we're adding a lot of layers but the layers don't necessarily abstract complexity in an ideal way. I think the refreshing Cache is a layer that makes sense (and I'd pull it into it's own module with its own tests). But I'm not sure we need more than one layer for the actual credentials provider implementation
|
|
||
| #[derive(Clone)] | ||
| pub(super) struct Cache { | ||
| margin: Duration, |
There was a problem hiding this comment.
can you add a doc comment explaining what this field is?
There was a problem hiding this comment.
I think building time based expiry into this cache explicitly might not be exactly what we want, but fine to keep it this way for now
There was a problem hiding this comment.
Added comment and renamed to buffer_time.
Hard to say right now, but I think alternate caching credentials provider implementations will use the cache the same way. Seems like a safe bet to defer refactoring until then.
| self.value.read().await.get().cloned() | ||
| } | ||
|
|
||
| pub async fn refresh<F, Fut>(&self, f: F) -> CredentialsResult |
There was a problem hiding this comment.
probably worth a doc comment explaining that the caller should ensure f is always the same and that there is no guarantee that this f will be called.
Also, I'm not sure about the name refresh here—it implies to me that it would reload load them. What about get_or_load? or get_or_fetch?
There was a problem hiding this comment.
Added doc comment and renamed to get_or_load.
| } | ||
|
|
||
| /// If the credentials are expired, clears the cache. Otherwise, yields the current credentials value. | ||
| pub async fn yield_or_clear_if_expired(&self, now: SystemTime) -> Option<Credentials> { |
There was a problem hiding this comment.
I think we'll probably need a way to also explicitly expire credentials.
There was a problem hiding this comment.
What is the use-case for this?
There was a problem hiding this comment.
credentials can become invalidated (eg. manually invalidated session credentials)
|
|
||
| const DEFAULT_REFRESH_TIMEOUT: Duration = Duration::from_secs(5); | ||
| const DEFAULT_CREDENTIAL_EXPIRATION: Duration = Duration::from_secs(15 * 60); | ||
| const DEFAULT_EXPIRATION_MARGIN: Duration = Duration::from_secs(10); |
There was a problem hiding this comment.
what about DEFAULT_CREDENTIAL_TTL? I think TTL is a little clearer than MARGIN here
There was a problem hiding this comment.
Time-to-live is kind of the opposite of what this is representing. I had chosen "margin" as in "safety margin". Chose to rename to buffer_time, since maybe that is slightly clearer, but I can't think of a good name for it.
There was a problem hiding this comment.
oh I totally misunderstood the purpose of this constant 🤕
This implements an initial lazy caching credentials provider, but doesn't attempt to add async runtime-agnostic timeouts or panic handling yet. Both of those will take a considerable amount of work still.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.