lockMayBeAvailable always returns FALSE

We have the same symptom when syncing config. Disabling Redis during config sync fixes the issue.

Tried the patch locally as our project is facing the same issue (as @claudiu.cristea said).
Patch solves the issue.
The issue has been reproduced using the predis library, but indeed should be the same with the phpredis backend.

As @danielnolde said, Redis returns NULL for non-existing keys (see https://redis.io/commands/get).

Current tests are not running correctly, see #3002634: RedisLockFunctionalTest is running with database lock implementations.
Even with that fixed, the current lock tests are extending core ones, which don't cover the case of a lock checked for availability when not acquired yet, it seems.
Attaching patch with a (temporary) test to outline the issue. Note that patch from #3002634-2: RedisLockFunctionalTest is running with database lock implementations needs to be applied.
I wrote temporary as the test written doesn't belong to that BrowserTest, it should be moved to a kernel for how it is written.
Or it could be ditched when #3002640: Improve test coverage for LockBackendInterface::lockMayBeAvailable() is in.

1. +++ b/src/Lock/PhpRedis.php
   @@ -107,7 +107,7 @@ class PhpRedis extends LockBackendAbstract {
        // In Drupal 7, this method treated the lock as available if the ID did
        // match. The database backend and test expects it to return FALSE in that
        // case, updated accordingly.
   
   +++ b/src/Lock/Predis.php
   @@ -101,7 +101,7 @@ class Predis extends LockBackendAbstract {
        // In Drupal 7, this method treated the lock as available if the ID did
        // match. The database backend and test expects it to return FALSE in that
        // case, updated accordingly.
   

   This comment looks odd.

2. +++ b/src/Lock/PhpRedis.php
   @@ -107,7 +107,7 @@ class PhpRedis extends LockBackendAbstract {
   +    return (FALSE === $value) || (NULL === $value);
   
   +++ b/src/Lock/Predis.php
   @@ -101,7 +101,7 @@ class Predis extends LockBackendAbstract {
   +    return (FALSE === $value) || (NULL === $value);
   

   Can we write this more simple, as return !$value? I have no idea if the value could be "" (empty string) or 0 and, if yes, such values have the same meaning (ie lock is available).

3. +++ b/tests/src/Functional/Lock/RedisLockFunctionalTest.php
   @@ -74,4 +74,21 @@ class RedisLockFunctionalTest extends LockFunctionalTest {
   +  public function testLockMayBeAvailable() {
   

   Yes, this belongs to #3002640: Improve test coverage for LockBackendInterface::lockMayBeAvailable(). I think we should:

   1. Attach here two cumulative patches that are including #3002634: RedisLockFunctionalTest is running with database lock implementations: one "test only" and a full one.
   2. Test the two patches in Travis CI and post the links here so that we know that the fix works.
   3. Postpone this on #3002640: Improve test coverage for LockBackendInterface::lockMayBeAvailable().
   4. When #3002640: Improve test coverage for LockBackendInterface::lockMayBeAvailable() is in, rework this by creating a Kernel test that just extends the new core one and sets-up the container to use Redis (similar to other redis kernel tests).

   How it sounds?

   EDIT: Or just add the same kernel test here and change it after the twin one goes in core.

Add related issues.

1. I agree that that comment should be rephrased.
2. I have thought of that too, but I don't have enough experience to make that call. In the D7 version the value was compared to the id too, but from the comment it looks like now it's not needed.
3. I agree with all but I would follow your last edit suggestion. Let's get the kernel here and remove it when and if the core patch gets in.

As @claudiu.cristea suggested I'm attaching the two cumulative patches that include #3002634: RedisLockFunctionalTest is running with database lock implementations:

• the test-only patch results https://travis-ci.org/brummbar/redis/builds/434320337
• the results with patch #2 applied too https://travis-ci.org/brummbar/redis/builds/434331536

Since this issue needs #3002634: RedisLockFunctionalTest is running with database lock implementations to be merged in order to run the tests, I'll avoid providing the non-cumulative patch for now, unless requested.
Putting up for a first round of review.
I'll address also the first comment from @claudiu.cristea tomorrow, I missed it now.

Actually it looks very painful to understand what has been added in the cumulative patch.
Attaching a patch with this issue code only (but still needs to be applied on top of the #3002634).

Noticed that I have wrongly used lock_a in the test testPersistentLock(). It needs to be lock1.
I'll fix it this evening.

#10.1:

I agree that that comment should be rephrased.

I think we should just drop it. It's unusual to have comment referring an older version of Drupal.

#10.2: Well, my understanding is that the Redis client is trying to get the object from the backend (which in our case is the lock entry). A valid lock would be a "non-empty" value. So, my common sense, whispers that returning !$value (or empty($value)) is OK. At least let's try and see what other reviewers or the module maintainer are saying. It would be a cleaner approach.

#11:

Actually it looks very painful to understand what has been added in the cumulative patch.
Attaching a patch with this issue code only (but still needs to be applied on top of the #3002634).

I know. The idea was just to run tests on Travis CI with cumulative patch in order to prove the fix works and then paste the test URLs here so that the module maintainer is able to see the "test only" failing and the full patch passing.

EDIT: Actually we're not blocked anymore on #3002634: RedisLockFunctionalTest is running with database lock implementations as we've been migrated to a Kernel test. So, just running the tests on Travis CI and pasting here the 2 links would be enough.

More review.

1. +++ b/tests/src/Kernel/RedisLockTest.php
   @@ -0,0 +1,62 @@
   +    parent::register($container);
   +
   ...
   +    parent::setUp();
   +
   +    $this->lock = $this->container->get('lock');
   

   Nit: When the method contains only few lines we can drop the empty line. It's more readable. (not mandatory)

2. +++ b/tests/src/Kernel/RedisLockTest.php
   @@ -0,0 +1,62 @@
   +    $container->register('lock', 'Drupal\Core\Lock\LockBackendInterface')
   

   Starting with PHP 5.5, the magic class name constant ::class was introduced. That is the preferred way to refer the class name as is more readable and if, for some reasons, the class namespace changes it would be easier to port the code.

3. +++ b/tests/src/Kernel/RedisLockTest.php
   @@ -0,0 +1,62 @@
   +    $this->assertInstanceOf('\Drupal\redis\Lock\\' . $redis_interface, $this->lock);
   

   Let's add a comment here, explaining what we're testing.

4. +++ b/tests/src/Kernel/RedisLockTest.php
   @@ -0,0 +1,62 @@
   +    $this->assertTrue($this->lock->lockMayBeAvailable('lock_a'));
   

   Actually, in #3002640: Improve test coverage for LockBackendInterface::lockMayBeAvailable() this will be part of LockTest::testBackendLockRelease(), probably as the first line there. So it makes sense to keep it as lock_a (?). Let's add also a TODO:

   @todo Remove this line when #3002640 lands.
   @see https://www.drupal.org/project/drupal/issues/3002640
   

Updating the patches to fix the remarks and two bad copy-pastes from my side.
Addressing first review from @claudiu.cristea:

• 10.1: indeed, removed completely the comment.
• 10.2: I also would like an opinion from the module maintainer. Theoretically the lock value is the lock ID generated through uniqid(), so a boolean cast should be enough.
• 11: the persistent lock test still require #3002634: RedisLockFunctionalTest is running with database lock implementations to be in for now. Core doesn't have a separated persistent lock test as the main difference is the persistence itself.
  Regarding the combined patch, I expressed myself badly. I absolutely agree with the combined patch approach :) I added also the final patch to understand better which code is in scope of this issue alone.

Now addressing comments from #14: agree on everything. Just on point 4, I changed the lock_a as it was a bad copy-paste from the kernel test. I changed the names to reflect the ones used in the parent test.

Cumulative patches test results:

1. Test only: https://travis-ci.org/brummbar/redis/builds/435594644
2. With fix: https://travis-ci.org/brummbar/redis/builds/435612999

Link to the PR in Github: https://github.com/brummbar/redis/pull/2 .

Great!

Sorry, I think that it should be postponed on #3002634: RedisLockFunctionalTest is running with database lock implementations.

Strange, I never saw these problems and I also can't make those tests fail. $value is FALSE when I'm debugging the test. PHP 7.2.10, php info says Redis 3.1.6 while info server in my redis server says 4.0.9. We're also using platform.sh in all our D8 projects and it also works fine for us there.

https://github.com/phpredis/phpredis#return-value-21 is also documented as returning FALSE.

I'm fine with committing this but it would be nice to understand what the change here is exactly?

@Berdir the error occurs only when using the Predis library.
When asking a non-existing lock, a get command will be executed on the key. By specifications of Redis, a NIL response will be returned.
The response will be $-1, which will be decoded by Predis \Predis\Connection\StreamConnection::read() function to NULL (line 328).
It seems that Phpredis instead returns FALSE.
So if you prefer, we can revert the changes in the Phpredis client and leave the null comparison only for the Predis client.

I think we can safely test for empty($value) in both.

Thanks, committed.

I did the review here but still left out from credits :(