Commit 0dbbc93
authored
Fix time parsing for limit cache (#45)
Fix a bug in the code that handles determining the timestamp for
values in the cache that's used to implement removing the oldest key
when the limit is reached. The bug is in parsing the time returned by
Redis, making it produce incorrect results when the leading portion of
the microseconds moves from 09 to 10. This tended to reveal itself as
flakiness in the `test_limit` unit test, which we were seeing around 1
in 200 runs.
The `TIME` operation in Redis returns the time as an array of two
strings, the first holding the number of seconds and the second
holding the number of microseconds. Importantly, the second string
has no leading zeroes. The Lua code parses this by concatenating
these strings together separated by a decimal point and then parsing
that as a number, which produces incorrectly ordered results when
faced with values like `("1234567", "999")` and `("1234567", "1000")`,
parsing them as 1234567.999000 and 1234567.100000, respectively,
meaning the second value will be expunged from the cache first despite
being inserted second.
Instead, parse each returned string as a number and then combine them
numerically, which produces the correct number in this situation.
I can't easily produce a test for this, because to test this
deterministically you have to control the timestamps Redis produces,
and the current testing setup uses a standalone Redis process. We
have a internal unit test that uses `freezegun` and an in-process
Redis fake to test it, but I didn't think you would want that level of
change to the testing configuration just to make the test for this
very narrow change work.1 parent 9cae0a2 commit 0dbbc93
1 file changed
+3
-1
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
64 | 64 | | |
65 | 65 | | |
66 | 66 | | |
67 | | - | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
68 | 70 | | |
69 | 71 | | |
70 | 72 | | |
| |||
0 commit comments