[9.x] Fix race condition in locks issued by the file cache driver

[bytestream]

Fixes #43627

Before:

root@fa0e2058d0ad:/app# php artisan app:test-locks --workers=2 --iterations=1000 --cache-driver=file
Running 1000 iterations with 2 workers using the cache driver 'file' for locking.
Process 1/2 started.
Process 2/2 started.
Process 1/2 finished.
Process 2/2 finished.
Process 1/2 started at 0 and ended at 1938.
Process 2/2 started at 0 and ended at 1932.
Counted only to 1938 instead of the expected 2000.

After:

root@fa0e2058d0ad:/app# php artisan app:test-locks --workers=2 --iterations=1000 --cache-driver=file
Running 1000 iterations with 2 workers using the cache driver 'file' for locking.
Process 1/2 started.
Process 2/2 started.
Process 1/2 finished.
Process 2/2 finished.
Process 1/2 started at 0 and ended at 1922.
Process 2/2 started at 26 and ended at 2000.
Successfully counted to 2000.

[taylorotwell]

Please explain how this changes anything? The FileStore already has an add method so it would already be falling into this conditional block anyways?

[bytestream]

@taylorotwell the problem is when $this->seconds = 0 it falls to the get / put combination which isn't atomic. The add method in the file driver is able to handle when $seconds = 0, and treats it as cache forever

[bytestream]

@taylorotwell 362fc83 is a cleaner way of handling it while maintaining BC

[taylorotwell]

Are other drivers not able to handle $seconds = 0?

[taylorotwell]

I also don't agree your second commit is necessarily better. The first was simpler?

[bytestream]

@taylorotwell CacheLock and HasCacheLock are only actually used by the file driver. They're a legacy from #35139 which originally added APC driver support as well, but was removed before it was merged. The simplest fix may just be the one that was originally suggested in #43661. If it were me though, I'd remove the HasCacheLock and rename CacheLock to FileLock. That's clearer and also follows the way other drivers are implemented. That's obviously a BC break though which is why I didn't go that route...

[taylorotwell]

Sorry - I'm not following at all but I don't have time to dig deeper at the moment.

1. Can other drivers not handle $seconds = 0?

2. Why is this not the simplest fix and easier?

[taylorotwell]

For what it's worth: I've updated the docs to not even show an example of not providing an expiration time (in seconds) to the lock methods.

[bytestream]

Can other drivers not handle $seconds = 0?

TLDR; not relevant as CacheLock is only used by the FileStore. To directly answer the question though:

• APC - ❌ , missing add method
• Array - 🆗 , missing add method, but ArrayLock handles $seconds = 0
• Database - 🆗 , providing $seconds = 0 to add() results in immediate expiry, but DatabaseLock uses expiry of 1 day (86400 seconds) if $seconds = 0
• DynamoDB - 🆗 , providing $seconds = 0 to add() results in immediate expiry, but DynamoDbLock uses expiry of 1 day (86400 seconds) if $seconds = 0
• File - 🆗 , expiry is set to 9999999999 in add()
• Memcached - 🆗 , 0 means never expire - https://www.php.net/manual/en/memcache.add.php
• Null - ❌ , missing add method
• Redis - 🆗 , providing $seconds = 0 to add() expires after 1 second, but RedisLock correctly handles $seconds = 0

Why is this not the simplest fix and easier?

Simplest fix in the sense that it's a 1 line change, but given CacheLock is only used by the FileStore it really makes sense to refactor the usage. #43661 is the simplest fix IMO without any additional refactoring, and has no breaking changes / consequences.