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

PromiseRegistry for caching async work #1770

Merged
merged 15 commits into from
Apr 16, 2020
Merged

PromiseRegistry for caching async work #1770

merged 15 commits into from
Apr 16, 2020

Conversation

markfields
Copy link
Member

@markfields markfields commented Apr 13, 2020

When cleaning up promise-function-async disables under the drivers dir, 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 (and 2 bugs) I played around a bit and came up with a way to refactor the tricky parts of that logic out.

See those 3 places updated to use PromiseRegistry in the draft PR #1713.

Once this is merged and the client packages are consuming fluid-common-utils version 0.15, I'll put out another PR to add those 3 usages.

To-do:

  • Fix build break (my local build has been pretty messed up...)
  • Write a million tests

@markfields
Copy link
Member Author

Well, I'm actually trying a different API surface, modeling after .NET's ObjectCache. I think it is easier to understand, based on the precedent of the .NET API and also some feedback on the draft PR that I've slowly digested.

Copy link
Contributor

@arinwt arinwt left a comment

Choose a reason for hiding this comment

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

Looks good. Depending on how it looked, I might have made the "indefinite" form a class on its own with the expiration forms in an class extending it, but definitely not necessary since this is already fairly simple.

@markfields
Copy link
Member Author

Update: Refactored GC into a helper class and wrapped asyncFn() in an async lambda to ensure it doesn't throw.

@markfields markfields merged commit 2ec4158 into microsoft:master Apr 16, 2020
@markfields
Copy link
Member Author

50% more LOC in the tests than the implementation itself 😛

@markfields markfields deleted the promise-registry branch April 16, 2020 19:22
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.

4 participants