Fix Redis TLL override on failure to add key

Fixes #56

CHANGELOG.md

@@ -1,6 +1,14 @@
 # Changelog
 
 
+## [1.5.3] - 2024-04-02
+### Fixed
+- Fixed Redis TLL override on failure to add key
+
+### Changed
+- Moved to PHPUnit >=10.x and attributes
+
+
 ## [1.5.2] - 2024-01-10
 ### Fixed
 - Stampede: removed unnecessary wait caused by lingering stampede key
@@ -349,3 +357,4 @@
 [1.5.0]: https://github.com/matthiasmullie/scrapbook/compare/1.4.9...1.5.0
 [1.5.1]: https://github.com/matthiasmullie/scrapbook/compare/1.5.0...1.5.1
 [1.5.2]: https://github.com/matthiasmullie/scrapbook/compare/1.5.1...1.5.2
+[1.5.3]: https://github.com/matthiasmullie/scrapbook/compare/1.5.2...1.5.3

src/Adapters/Redis.php

@@ -228,14 +228,66 @@ public function add(string $key, mixed $value, int $expire = 0): bool
         }
 
         /*
-         * I could use Redis 2.6.12-style options array:
-         * $this->client->set($key, $value, array('xx', 'ex' => $ttl));
-         * However, this one should be pretty fast already, compared to the
-         * replace-workaround below.
+         * Redis supports passing set() an extended options array since >=2.6.12
+         * which allows for an easy and 1-request way to replace a value.
+         * That version already comes with Ubuntu 14.04. Ubuntu 12.04 (still
+         * widely used and in LTS) comes with an older version, however, so I
+         * want to support that too.
+         * Supporting both versions comes at a cost.
+         * I'll optimize for recent versions, which will get (in case of add
+         * failure) 1 additional network request (for version info). Older
+         * versions will get 2 additional network requests: a failed add
+         * (because the options are unknown) & a version check.
          */
+        if ($this->version === null || $this->supportsOptionsArray()) {
+            $options = ['nx'];
+            if ($ttl > 0) {
+                /*
+                 * Not adding 0 TTL to options:
+                 * * HHVM (used to) interpret(s) wrongly & throw an exception
+                 * * it's not needed anyway, for 0...
+                 * @see https://github.com/facebook/hhvm/pull/4833
+                 */
+                $options['ex'] = $ttl;
+            }
+
+            // either we support options array or we haven't yet checked, in
+            // which case I'll assume a recent server is running
+            $result = $this->client->set($key, $value, $options);
+            if ($result !== false) {
+                return $result;
+            }
+
+            if ($this->supportsOptionsArray()) {
+                // failed execution, but not because our Redis version is too old
+                return false;
+            }
+        }
+
+        // workaround for old Redis versions
+        $this->client->watch($key);
+
+        $exists = $this->client->exists('key');
+        if ($exists) {
+            /*
+             * HHVM Redis only got unwatch recently
+             * @see https://github.com/asgrim/hhvm/commit/bf5a259cece5df8a7617133c85043608d1ad5316
+             */
+            if (method_exists($this->client, 'unwatch')) {
+                $this->client->unwatch();
+            } else {
+                // this should also kill the watch...
+                $this->client->multi()->discard();
+            }
+
+            return false;
+        }
+
+        // since we're watching the key, this will fail should it change in the
+        // meantime
         $this->client->multi();
-        $this->client->setnx($key, $value);
-        $this->client->expire($key, $ttl);
+
+        $this->client->set($key, $value, $ttl);
 
         /** @var bool[] $return */
         $return = (array) $this->client->exec();

tests/AbstractKeyValueStoreTestCase.php

@@ -265,6 +265,20 @@ public function testAddFail(): void
         $this->assertEquals('value', $this->testKeyValueStore->get('test key'));
     }
 
+    public function testAddWithExpirationFail(): void
+    {
+        $this->testKeyValueStore->set('test key', 'value', 1);
+        $return = $this->testKeyValueStore->add('test key', 'value-2', 5);
+
+        // wait 2 seconds; this ensures that we did not touch the original value,
+        // which will have expired
+        // @see https://github.com/matthiasmullie/scrapbook/issues/56
+        sleep(2);
+
+        $this->assertFalse($return);
+        $this->assertEquals(null, $this->testKeyValueStore->get('test key'));
+    }
+
     public function testAddExpired(): void
     {
         $return = $this->testKeyValueStore->add('test key', 'value', time() - 2);