RFC - PromiseRegistry for caching promises that should run only once #1713
Conversation
from 0.14.0 to 0.15.0
…amework into promise-cache
| }); | ||
| } | ||
|
|
||
| cache.sessionStorage.put(joinSessionKey, cachedResultP, 3600000); |
There was a problem hiding this comment.
Bug # 1: This wouldn't actually cancel the existing timeout, so the comment above "we put the same result again with updated time" was not properly implemented. Fixed it in PromiseRegistry.
| { headers }); | ||
| return response.data; | ||
| }, | ||
| () => false, // unregisterOnError |
There was a problem hiding this comment.
Bug # 2 - This should probably not be left () => false. That means if resolving the URL fails, you'll never be able to get it resolved properly again.
markfields
requested review from
vladsud,
arinwt,
anthony-murphy,
ChumpChief,
skylerjokiel and
curtisman
and introduce a config type for self-documenting syntax
| public async register( | ||
| key: string, | ||
| asyncFn: () => Promise<T>, | ||
| expiryTime?: number, |
There was a problem hiding this comment.
I'm not a big fan of integrating time like this. i'd rather leave that up to the caller to schedule the unregister
There was a problem hiding this comment.
I thought it was worthwhile to keep (this concept existed on ISessionCache in odspCache.ts), especially to provide the semantics around refreshExpiryOnReregister which I found deceptively tricky to implement.
|
did you look at LazyPromise? That has delays running, and only runs once. I think that may work in most cases. |
There was a problem hiding this comment.
@anthony-murphy - Thanks for the review, really appreciate your comments/questions!
As for this:
did you look at LazyPromise? That has delays running, and only runs once. I think that may work in most cases.
Something this seems to encapsulate is map management. Seems like some of these cases could be solve with a function like getOrAdd<TKey,TValue>(map:Map<TKey,TValue>, key: TKey, add:()=>TValue): TValue
I had similar debates with myself - whether to just update these callers to use a wrapper like LazyPromise, or to provide the map implementation too. What swayed me was this bug - It's not just as simple as writing getOrAdd (which btw is basically what I called register in my first draft of this!). And then finding other useful capabilities like expiration that callers can offload to a utility like this felt affirming.
Although, I could see the argument that it's doing too many things and is trying too hard to provide a generic solution, so happy to keep discussing to find a good middle ground.
| public async register( | ||
| key: string, | ||
| asyncFn: () => Promise<T>, | ||
| expiryTime?: number, |
There was a problem hiding this comment.
I thought it was worthwhile to keep (this concept existed on ISessionCache in odspCache.ts), especially to provide the semantics around refreshExpiryOnReregister which I found deceptively tricky to implement.
| } | ||
|
|
||
| // Schedule garbage collection if required | ||
| if (expiryTime) { |
There was a problem hiding this comment.
Move this up into the if (handle === undefined), since it doesn't need to be called if the handle already existed? Or invert the conditional and return early if the handle already existed?
There was a problem hiding this comment.
Actually this needs to stay outside - if the key's already registered we may need to refresh the expiration on it.
| */ | ||
| export interface PromiseRegistryConfig { | ||
| refreshExpiryOnReregister?: boolean, | ||
| unregisterOnError?: (e: any) => boolean, |
There was a problem hiding this comment.
Do we need an unregisterOnError? Seems that if we default to false, then users of the registry could elect to do their own pretty easily:
promiseRegistry.register(k, fn)
.catch(() => { promiseRegistry.unregister(k); });
There was a problem hiding this comment.
I do not think this can be easily done in all cases by callers.
Scenario - we want to coalesce and have only one outstanding request, but once it fails, we want to retry.
We do not want second pending request to be removed by one of the first users who are processing first failed request.
In reply to: 404434391 [](ancestors = 404434391)
There was a problem hiding this comment.
@vladsud - Is this the case you're describing?
1 - Call register, add .catch. Starts the request.
2 - Call register, add .catch. Request from 1 still in progress
3 - Request fails, two .catch continuations are on the microtask queue
4 - One .catch callback runs: unregister
5 - New call to register - starts a second request which will succeed
6 - Second .catch callback runs: wrongly unregister the 2nd request.
This would require 5 jumping into the microtask queue from some other existing microtask as of 3 - I guess this is possible?
Anyway, if it's this hard to reason about, seems useful to do it the safer way of only adding the .catch internally (and then callers can't forget as in UrlResolver).
|
Thanks everyone for the great feedback! I've addressed everything here, and just opened a new PR with only the utils changes (#1770). Once I write tests I'll plan on merging that, feel free to add any additional feedback on that PR. |
When cleaning up
promise-function-asyncdisables under thedriversdir, there were two cases I left in place, because the functions were about caching/retrieving Promises, and needed to stay synchronous to discourage accidentally awaiting the promises before the caching was resolved properly. After finding a third similar pattern I played around a bit and came up with a way to refactor the tricky parts of that logic out.I found 2 bugs in doing so, so that's a good sign that this is a worthwhile abstraction to build. I think there may be many other places in the code that could use this as well, I found a bunch of hits when doing a text search for
Map<.*, Promise.The capabilities of
PromiseRegistrymeet the needs of the 3 examples I found so far, what do folks think of the API/implementation? I was also considering the nameConcurrentCache, and renaming the functions accordingly. Very open to other naming suggestions. And thank you to @arinwt for pairing with me on it and helping land on the design.This is a draft PR because I need to check in
PromiseRegistryfirst to get the changes into the publishedfluid-common-utilspackage. In that PR I'll write a million tests to make sure it's working properly :)