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

Add expirable LRU finalizer to fix expirable LRU's goroutine leak #161

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

canoriz
Copy link

@canoriz canoriz commented Nov 19, 2023

Apply the trick used in go-cache to solve go routine leak when dynamically creating many expirable-LRU cache. #159

The deleteExpired go routine will be terminated by finalizer at some garbage collection time in the future, although not immediately after cache goes out of scope.

This trick is wrapping the lru type and does not affect exposed APIs. If LRU.Close() method will be re-added in the future, a small change is needed to prevent double close over c.done. But for now, the only caller of close(c.done) is the finalizer, and the finalizer must be called only once, so it's OK for now.

@hashicorp-cla
Copy link

hashicorp-cla commented Nov 19, 2023

CLA assistant check
All committers have signed the CLA.

@canoriz
Copy link
Author

canoriz commented Nov 29, 2023

Could you please take a look at these codes when you have some time? @paskal

Copy link
Contributor

@paskal paskal left a comment

Choose a reason for hiding this comment

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

Clever idea. I like it.

@paskal
Copy link
Contributor

paskal commented Dec 18, 2023

I've had time to think about it, and now I think it's better to make it simply a "Close" method rather than destructor magic. Using a destructor in Go feels too clever.

@canoriz
Copy link
Author

canoriz commented Dec 23, 2023

I agree close() is better. But before close is re-added, create cache (especially inside a for loop) leaks go routines.

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.

3 participants