fix: Memcached decrement() gives the wrong sign on a missing key - #10512
fix: Memcached decrement() gives the wrong sign on a missing key#10512mdalikadar wants to merge 3 commits into
Conversation
|
Hi there, mdalikadar! 👋 Thank you for sending this PR! We expect the following in all Pull Requests (PRs).
Important We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md Sincerely, the mergeable bot 🤖 |
MemcachedHandler::decrement() passed $offset as Memcached::decrement()'s initial_value, which is used as-is (not decremented from) when a key doesn't exist yet. That made a fresh key end up at +$offset instead of the -$offset every other cache handler (File, Redis, Predis) produces. Memcached counters are unsigned, so they can't hold a negative initial value the way the other handlers effectively can. Use 0 instead (also Memcached::decrement()'s own default), so a fresh key at least stops going the wrong direction. Updates the existing MemcachedHandlerTest::testDecrement() expectation and adds a changelog entry.
d6322dc to
daed2b1
Compare
| // Fall back to Memcached::decrement()'s own default of 0 instead | ||
| // of $offset, so a new key no longer starts positive. | ||
| return $this->memcached->decrement($key, $offset, 0, 60); |
There was a problem hiding this comment.
Could we reframe this as a Memcached-specific correction rather than a cross-handler consistency fix, and document the unsigned/saturating counter limitation in the user guide?
There was a problem hiding this comment.
Good point, done. Reworded the comment so it's just about Memcached's own unsigned/saturating counters, no more comparing it to the other handlers. Also added a note in the user guide under decrement() explaining that missing keys start at 0 and existing counters can't go below 0 either.
| - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. | ||
| - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden). | ||
| - **Logger:** Fixed a bug where interpolating a log message with array or non-stringable context values could raise PHP warnings or errors. | ||
| - **Cache:** Fixed a bug where ``MemcachedHandler::decrement()`` on a non-existent key returned a positive value instead of ``0``, the opposite sign of what ``FileHandler``, ``RedisHandler``, and ``PredisHandler`` return. |
There was a problem hiding this comment.
This wording still suggests consistency with File, Redis, and Predis, but those handlers return -$offset, whereas this change returns 0. Could we describe the actual correction more directly?
- **Cache:** Fixed ``MemcachedHandler::decrement()`` initializing a non-existent counter to the positive offset. Missing counters are now initialized to ``0``, reflecting Memcached's unsigned, saturating counter semantics.There was a problem hiding this comment.
Makes sense, you are right that it was not quite accurate. Used your wording for the changelog entry.
| // A key that doesn't exist yet starts at 0, not at the offset | ||
| // (Memcached counters are unsigned, so it can't start negative). | ||
| $this->assertSame(0, $memcachedHandler->decrement(self::$key3, 1)); |
There was a problem hiding this comment.
Could we use an offset greater than 1 and also verify the stored value? That would make the regression clearer and prove that 0 is used as the initial value rather than merely matching a special case.
$this->assertSame(0, $memcachedHandler->decrement(self::$key3, 5));
$this->assertSame(0, $memcachedHandler->get(self::$key3));There was a problem hiding this comment.
Good catch, offset of 1 did not really prove anything. Changed it to 5 and added a get() check on the stored value like you suggested.
2 participants
Fixes #10510 (#10510)
Small follow-up to that issue:
MemcachedHandler::decrement()was passing$offsetas the initial value for a key that doesn't exist yet, which madea fresh key end up at
+$offsetinstead of-$offsetlike every othercache handler (File, Redis, Predis) gives you.
I swapped it to use
0instead, since Memcached counters are unsigned andcan't actually go negative — so
0is the closest sane starting point,and it matches what
Memcached::decrement()already defaults to on itsown.
What I changed:
MemcachedHandler::decrement()now passes0instead of$offsetasthe initial value