Cache concurrency issue

[ambienthack]

I experienced hard-to-reproduce errors in an app that uses php-annotations. Looks like the problem is that php-annotations cache cannot handle some concurrency scenarios.

Simplified caching workflow is: check if entity is in the cache (check if cached file exists) a. if file exists, a.1 include the file. a.2. end of workflow b. if file doesn't exist, b.1. create empty cache file b.2. lock the file b.3. write cache data b.4. end of workflow

Now consider that the cache is empty & we make two almost simultaneous requests that cause creation of the same cache entities.

Request I:

1. check if cached file exists (no)
2. create empty cache file
3. lock the file (the file is still empty) ...

Request II

1. check if cached file exists (yes)
2. include the file (but the file is still empty)

The solution can be to write to temp file, and then move it into the cache.

[aik099]

The proposed solution with temp file looks viable IF case, when cache exists, but is outdated is handled the same way if cache doesn't exist. Please send a PR.

Can you please explain when this could happen in real life?

[mindplay-dk]

The call to file_put_contents() specifies an exclusive lock (LOCK_EX) while writing.

So yes, there's a theoretical but extremely small chance that two threads end up taking turns writing identical contents to the same file.

We're optimizing for the likely case where there's no race condition - and we're already handling the extremely unlikely case where two threads try to write at the same time by using LOCK_EX to ensure we don't end up with an empty file or duplicate file contents.

@ambienthack a more complex strategy could address this, sure - but what for exactly? an extremely marginal performance gain in the case where two threads perform a cache refresh simultaneously? doesn't really seem worth while.

[ambienthack]

@mindplay-dk Please, correct me, if I'm mistaking. To my understanding, the situation I described leads to an error, not double write to file. Second thread thinks that there is a cache entity, includes it & tries to treat it as if there were some valid php there. But there is an empty file.

===

In my case the error was rather frequent. It occurred straight after deployment of new app build, I'd say, in 30-50% of deployments. Then cache stabilised & the error went away by itself.

I think the app architecture played some role in emerging of the error. It is a REST server. After the deployment, when you load a frontend page, it makes a lot of requests to the server. Those requests are queued by browser and it turns out that it fires bunches of requests almost simultaneously.

[mindplay-dk]

Second thread thinks that there is a cache entity, includes it & tries to treat it as if there were some valid php there. But there is an empty file.

If that's true, that would mean PHP doesn't respect it's own locking mechanism.

I guess it's quite possible that it doesn't respect it's own locking mechanism. It's PHP after all 😆

Since no one else has had this issue, it'll likely be difficult for anyone but you to reproduce, so I'd like to suggest you start by simply hacking the library to see if you're able to cure the issue?

Let's see, something like this maybe?

    public function store($key, $code)
    {
        $path = $this->_getPath($key);

        $content = self::PHP_TAG . $code . "\n";

        $temp_path = $path . "." . uniqid('', true);

        if (@\file_put_contents($temp_path, $content, LOCK_EX) === false) {
            throw new AnnotationException("Unable to write cache file: {$path}");
        }

        if (@\chmod($temp_path, $this->_fileMode) === false) {
            throw new AnnotationException("Unable to set permissions of cache file: {$path}");
        }

        @rename($temp_path, $path);
        @unlink($temp_path);
    }

Try pasting over the method in AnnotationCache and let us know what happens?

Note that I'm deliberately suppressing @rename and omitting a check for success - if there's a race, the winning thread will manage to put the file in the right location, and the @unlink is for the losing thread to clean up the useless extra copy of the cache-file it generated.