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

Revert change to how expiration timings to calculated. #403

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

Conversation

gurudesu
Copy link

@gurudesu gurudesu commented Aug 7, 2024

When testing upgrading from v3.0.2 to v3.1.0, I've noticed a small bug impactful (to me) change around how expirations are calculated.

If you set a LifeWindow of 1s, add an entry, and wait 1100ms, the entry does not expire. Testing it a bit more, I've discovered that it actually expires if you wait 2s.

You can see this regression in code when comparing the old logic here with the new logic here.

For convenience, old logic:

if currentTime-oldestTimeStamp >= s.lifeWindow {
}

new logic:

return currentTimestamp-oldestTimestamp > s.lifeWindow

This change reverts that logic to the old behavior.

Comment on lines 1180 to +1183
name: "Expired",
clock: 5,
wantData: value,
wantResp: Response{},
wantResp: Response{EntryStatus: Expired},
Copy link
Author

@gurudesu gurudesu Aug 7, 2024

Choose a reason for hiding this comment

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

This test case conflicts with the test name - if the name is Expired maybe it should be expected to be expired.

This patch has that desired behavior.

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.

1 participant