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

Draft of MapWithExpiration class #9420

Closed
wants to merge 1 commit into from

Conversation

markfields
Copy link
Member

Not planning to merge this, just sharing code

@NicholasCouri - My other idea for fixing #4500, based on feedback in your original PR that exported GarbageCollector, is to refactor GarbageCollector to a class MapWithExpiration that extends Map and handles the expiration internally. This branch shows my quick implementation of it and usage in the ODSP cache code. If you update it to take a variety of expiration policies not just expiryMs then it should work for PromiseCache as well.

If this approach is appealing to you, probably best to shop it around and get buy-off from the team to add a utility like this in common-utils before doing more coding on it. But if folks agree, then you can write it properly (I'm sure I have bugs) and test it and then finally replace the two GarbageCollectors with it once it's in.

@github-actions github-actions bot added area: driver Driver related issues area: odsp-driver labels Mar 9, 2022
class MapWithExpiration<TKey, TValue> extends Map<TKey, TValue> implements IDisposable {
public disposed: boolean = false;
private readonly expirationTimeouts = new Map<TKey, ReturnType<typeof setTimeout>>();

Copy link
Contributor

@NicholasCouri NicholasCouri Mar 9, 2022

Choose a reason for hiding this comment

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

I was thinking more about it and I like it - We could simply rename the PromiseCache's GarbageCollector to MapWithExpiration + add the IDisposable ?

@markfields markfields closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants