Fix locking

[FinBoWa]

First steps on trying fixing the issue #17

Also there will be a pull request to the cache manager side also as that needs to use locking as well.

Seems to work with the redis provider but there is sue with lock carbage collection if the provider doesn't support setting TTL's for the keys.

Will tweak this more next week and squash the commits to a single one.

The cc for the lock keys isn't high priority for us as we use redis 100% so not sure will I be able to source time to tweak that.

[kinglozzer]

This looks promising, I think flushByName() would also need to use locking to prevent similar issues during regular cache invalidation (via writes)

[FinBoWa]

@kinglozzer you are right thought i might have some reason to skip it when I created the locking.. Can't frankly remember it anymore :) .

That might need a while loop against the existence of the lock so it could lock it to it self as soon as possible.

[stecman]

This looks like it might help a little, but as your locking isn't atomic it's not going to completely solve the issue - it just narrows the window where a race condition can occur, roughly by the duration of an array_key_exists call.

I'm all for small improvements, but I'm hesitant to accept this with the extra clean up that will be needed for backends that don't support a TTL, given that it doesn't completely fix the problem here.

With the current way CacheInclude works, a couple of other options might be:

• Add locking to Doctrine Cache. I'd hazard a guess that the maintainers won't be interested in adding this, since asking for locking means we're using their cache like database, but we could always ask if they'll accept a PR for that.. The caching interfaces in PSR-6 and PSR-16 also don't have locking.

• Implement a separate mechanisim for tracking stored keys that supports locking. Support a bunch of backends that support locking - file-based (via flock) and Redis at least. The downside of this is that we have to maintain it, but what CacheInclude is doing here doesn't fit into an off the shelf library.

• Add wrappers or subclasses of the Doctrine\Cache providers that add support for locking. This isn't ideal, but it might be better than building and maintaining similar functionality from scratch as above. With the way providers get their backends, it would be pretty easy to do this without depending on any internal implementation details.

[FinBoWa]

I'd just wish that the cache drivers would support getting keys with wildcards.

That would help also.

Limiting to only few supported cache types would of course ease on the pain on maintaining that support lists or wildcards.

After featuring this additional tweak: Sohova/silverstripe-cacheinclude@c878669 we have being using this against redis servers without any major issues.

[kinglozzer]

Just to keep this rumbling along: I’ve been running a variation of this PR (using flock()) on a high-traffic site for a few months now with no issues kinglozzer@a0bdf91. It essentially treats cache writes as non-essential (so if the lock is in-use, it skips writing) and cache deletions as essential (it waits for the lock to be free, and uses the lockfile to blocks any writes until it has finished deleting).

[FinBoWa]

@kinglozzer much simpler than ours.. Can't remember why i wanted to force the lock file to the cache also.

[FinBoWa]

@kinglozzer I did add this also to prevent races if flushling caches from the manager https://github.com/heyday/silverstripe-cacheinclude-manager/pull/2/files or it seemed that I needed to.

[kinglozzer]

Ah, I’m not running the cache manager module so haven’t made any changes to that