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

Expirable LRU can return nil value with ok = true #150

Closed
benweint opened this issue Aug 16, 2023 · 3 comments · Fixed by #156
Closed

Expirable LRU can return nil value with ok = true #150

benweint opened this issue Aug 16, 2023 · 3 comments · Fixed by #156
Assignees

Comments

@benweint
Copy link

benweint commented Aug 16, 2023

When a value is requested from the cache right after it has expired, but before the expiration reaper has had a chance to remove it, the new expirable LRU can return a nil value but a value of true for the ok return value.

I can't tell whether this is intended or not, but it was surprising to me (I'm used to the contract being that ok == true implying that the value would be non-nil), and doesn't appear to be documented anywhere that I could find.

Here's a repro case (the timing works on my machine, but might need some tweaking or more effort simulating lots of reads around the time of expiry to work on yours):

package main

import (
	"fmt"
	"github.com/hashicorp/golang-lru/v2/expirable"
	"time"
)


func main() {
	cache := expirable.NewLRU[string, *string](10, nil, 1 * time.Second)

	start := time.Now()
	val := "value"
	cache.Add("k", &val)

	for {
		result, ok := cache.Get("k")
		if ok && result == nil {
			fmt.Printf("got result = nil, ok = true after %v\n", time.Since(start))
			return
		}
	}
}

When I run this I get:

❯ go run main.go
got result = nil, ok = true after 1.000000209s
@yuuuuu422
Copy link

I think this code causes the bug:

if time.Now().After(ent.ExpiresAt) {

I don't know why a judgment statement was added here, in fact, I'll remove it or change it to return value, false

@debugger22
Copy link

This hasn't been released?

@debugger22
Copy link

My bad, just saw the 2.0.7 release

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 a pull request may close this issue.

4 participants