[6.x] Introduce a locking mechanism for the array cache driver

[timacdonald]

This PR introduces a locking mechanism to the array cache driver, for testing.

It is currently not possible to substitute the array cache driver in tests for any driver that supports locking if your test suite / app relies on the locking functionality.

This PR changes that and is something I've wanted for on projects I work on.

This allows me to run the test suite without having to have a lock supporting cache driver running / setup on my machine - which is nice coming into a project and working on parts of a project that don't interact with the cache locking mechanism because it means I can setup and digging into code faster - and it also means a test environment can be setup without requiring a "real" driver like Redis / memcached to be setup and configured.

It's also nice that once you do go down the path of introducing cache locking into your project, you don't have to also change you testing driver. It just continues to work as expected. Related: #29161

I'm happy to package this up if it isn't something you want to add / maintain in the framework.

I'd love if someone could triple check the expiration tests to make sure my logic is sold there, and I don't need to shift the boundary forward a microsecond

Not that is really matters, because the in memory cache drivers are so fast...but this is faster than, say, memcached. But, like, both are so quick...it isn't going to have a speed impact because the difference is negligible. I just thought I'd mention it.

[taylorotwell]

Nice work 👍

[driesvints]

@timacdonald nice work dude!

[timacdonald]

Thanks! Hopefully it'll be useful to others as well :)

[barryvdh]

Nice! It always bothered me that I couldn't test it locally also.

Could we add locking to other drivers as well, even though they might not be 100% atomic? To make it possible to use the same codepath locally, when not using redis for example.

[Spartaques]

I spent a lot of time for understanding what's going on and when i understood that it's all because array driver not support locking i was very hungry. But for now it's not a problem. Good work)

[Spartaques]

@timacdonald

[timacdonald]

@Spartaques great to hear it'll be handy for you!

[paulandroshchuk]

@timacdonald thanks for this amaizing PR, that's something I've been wanting to for some time.

I just faced a small issue when releasing a released lock. Seems like right now it just simply unsets it from the store which makes it throw an Undefined index {index} exception if the lock's key is missing in the store.

However if we try to release a released/incorrect lock using Redis, it won't throw any exception because Redis won't throw it. (not checked whether it throws with other drivers such as DynamoDB, etc.)

Also, I'm thinking that it will also throw an exception if we're trying to release a lock aquired by another parallel process.

So, what do you think if we release the lock only if it exists within the array store?

public function forceRelease()
{
    if ($this->exists()) {
        unset($this->store->locks[$this->name]);
    }
}

[paulandroshchuk]

@timacdonald I can make a PR for that, just wanted to validate the idea before I do it.

[GrahamCampbell]

Please target your PR at 7.x, or if there are breaking changes that can't be reviewed tomorrow morning, master. Laravel 7.0.0 will be released tomorrow.

[timacdonald]

@paulandroshchuk i’d love to see the exact error from your test suite. It was my understanding that unset() doesn’t care if the array key exists or not. So maybe it is something else causing the issue. Eithe way, a stacktrace would be great to help you debug this.

See: https://3v4l.org/IhaC2

[timacdonald]

Re: parallel process - I don’t think that is a concern as the driver is only meant for testing. I also thought that parallel test runners all create their own instance of the container and it’s bound service classes, so there shouldn’t be any clashes between threads

[paulandroshchuk]

@paulandroshchuk i’d love to see the exact error from your test suite. It was my understanding that unset() doesn’t care if the array key exists or not. So maybe it is something else causing the issue. Eithe way, a stacktrace would be great to help you debug this.

See: https://3v4l.org/IhaC2

@timacdonald

My bad, unset() shouldn't really throw any exceptions.

I just revisited it and let's take a look at ArrayLock#70. When checking if the lock is owned by the current process it just checks the lock's ['owner'], and if the store doesn't have any lock (which might have been previously released) with the provided name, it will throw an "Undefined index" exception.

So, I think that we'll need to check whether the lock exists in the release() method:

if (! $this->exists()) {
    return false;
}

if (! $this->isOwnedByCurrentProcess()) {
...

[timacdonald]

Ahh, nice yea I can see how we might run into that. I think that makes sense but would be good to have a failing test that we can PR with the fix. Did u wanna put one together or would u like me to help you write one? More than happy to help out.

I can probably help with providing a failing test to you this afternoon and you can add it to the fix PR.

[paulandroshchuk]

I can probably help with providing a failing test to you this afternoon and you can add it to the fix PR.

That would be awesome! Thanks @timacdonald

[timacdonald]

Cool. I didn’t see ya response last night but I’ll whip one up this arvo and ping ya with it.

[timacdonald]

@paulandroshchuk here is a failing test for your PR: timacdonald@226ea07?diff=unified

You can pull that commit into your PR or just copy and paste the code into your PR. Whatever works best for you is totally cool with me. Let me know if I can help in any other way with it.