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

Incorrect cache duration due to a precision issue. #118

Closed
Stumble opened this issue Dec 29, 2022 · 7 comments
Closed

Incorrect cache duration due to a precision issue. #118

Stumble opened this issue Dec 29, 2022 · 7 comments

Comments

@Stumble
Copy link
Contributor

Stumble commented Dec 29, 2022

Because internally the granulariry of expiration time is second, when the physical time is like xx::yy::zz.950 (950 ms passed for the second), and if we set a value with expiration of 1 second, the actual cache duration is only 50ms, instead of 1 second.

A test case to reproduce the issue:

diff --git a/cache_test.go b/cache_test.go
index 6082299..62e43b3 100644
--- a/cache_test.go
+++ b/cache_test.go
@@ -43,6 +43,24 @@ func (mock *mockTimer) NowCallsCount() int {
        return int(atomic.LoadUint32(&mock.nowCallsCnt))
 }
 
+func TestFlakyDuration(t *testing.T) {
+       for {
+               cache := NewCache(1024)
+               err := cache.Set([]byte("key"), []byte("val"), 1)
+               if err != nil {
+                       panic(err)
+               }
+               time.Sleep(200 * time.Millisecond)
+               v, err := cache.Get([]byte("key"))
+               if err != nil {
+                       panic(err)  // <- will failed here when the time is right:)
+               }
+               if string(v) != "val" {
+                       t.Error("test failed")
+               }
+       }
+}
+
:

Shall we consider to use milliseconds internally?

@coocood
Copy link
Owner

coocood commented Dec 29, 2022

Uint32 is not large enough to include milliseconds.

@Stumble
Copy link
Contributor Author

Stumble commented Dec 29, 2022

Understood. Is it possible to change it to uint64?

@coocood
Copy link
Owner

coocood commented Dec 29, 2022

Understood. Is it possible to change it to uint64?

It is possible but maybe not worth to do so.
The memory usage would increase 8 bytes per entry.

@Stumble
Copy link
Contributor Author

Stumble commented Dec 29, 2022

Make sense. Sounds like a reasonable trade-off. Should it be documented somewhere?

Btw why is it 8 bytes more instead of 4 bytes more?

@coocood
Copy link
Owner

coocood commented Dec 29, 2022

Each entry stores two Uint32 values, accessTime and expireAt.

Yes, it could be documented, would you like to send a PR for it?

@Stumble
Copy link
Contributor Author

Stumble commented Dec 29, 2022

sure thing: #119

@coocood
Copy link
Owner

coocood commented Dec 30, 2022

Thank you for the help!

@coocood coocood closed this as completed Dec 30, 2022
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

No branches or pull requests

2 participants