lock_acquire might be checking the wrong thing when extending a lock lease

PoC code:

For process 1:

$lock = lock_acquire('test', 10);
var_dump($lock);
sleep(15);
echo "Lock should be gone\n";
sleep(10);
$lock = lock_acquire('test');
var_dump($lock);
sleep(30);

Process 2:

$lock = lock_acquire('test');
var_dump($lock);
sleep(30);

In order to test, run the process 1 script. As soon as you get "lock should be gone" output, start the second one in a separate terminal. You would hope that because process 2 claimed the lock after it expired in process 1, process 1 would not be able to extend its lock, but what happens is process 1 thinks it got the lock:

Process 1:

bool(true)
Lock should be gone
bool(true)

Process 2:

bool(true)

The attached patch makes it so that the first process cannot extend a lock which has been claimed by another process, and so process 1 test code output looks like this:

bool(true)
Lock should be gone
bool(false)

I added a couple of extra parentheses for clarity.

Note also: as with #2379897: lock_release does not check for lock_id : it's actually a forced lock break, the behaviour with this patch more closely emulates the core SQL lock.inc behaviour in that it does not allow a process to extend a lock that was already claimed by another process. This is the output from process 1 when running with core lock.inc (I added the "DB lock" output into includes/lock.inc to prove it was being executed):

DB lock
bool(true)
Lock should be gone
DB lock
bool(false)

Or course, there's also the possibility that I'm just reading this wrong and the unused $result is a red herring. If the purpose of that is simply to establish that there is a non-FALSE result (in which case, $result can be removed altogether) from that get() call AND the check at the end is simply to establish "I believe I claimed the lock earlier in this process" before extending the lock, then the only problem that remains is the race condition between the get() and the set(). But then, there is already the isset() earlier in the line to do that, so the last check doesn't help.

However it comes out, this could use a comment to ensure that the intention is obvious.

Many lock implementations don't attempt to extend an existing lock, and so don't have to deal with this issue. The problem with memcache in this case is that we can't impose multiple conditions on what we're attempting to set, which core's lock.inc can, due to MySQL. I'm wondering if there's a slightly more elaborate memcache-based data structure that would allow this to work, using multiple keys then, or whether we'd need a "micro-mutex" (can't think of a better word) lock around the non-atomic sections (anywhere that has a separate get and set operation). That micro-mutex would have no lock ID at all so that it's fully shared between all processes, and would live only microseconds in order to protect the attempt to get/set the actual lock we're interested in. This would be needed to do proper checked release calls (see #2379897: lock_release does not check for lock_id : it's actually a forced lock break) as well as extending the lock as discussed here.

I suspect that this may be the reason this test fails: http://cgit.drupalcode.org/memcache/tree/tests/memcache-lock.test#n68