PsrCachedReader::fetchFromCache might return null instead of array under high request rate

[dshatovskiy]

Hi everyone.

We are fandom and very rare error
request.CRITICAL: Uncaught PHP Exception TypeError: "Doctrine\Common\Annotations\PsrCachedReader::fetchFromCache(): Return value must be of type array, bool returned" at ...

This usually happens on applications with the high request rate and some of such caches are empty.
The working scenario - new code release which requires to flush caches in order to new changes took in place.
When we debug the problem - it is appeared, when some of the requests hit that method and retrieve CacheItem, it has isHit property "true" but value "null".
Obviously, we made sure nothing overrides this cache items with null values.
Therefore we made conclusion this is "race condition" problem

We tried different approaches as a hotfix:

1. Allowing null values in this method doesn't work for us because it triggers error in some other vendor code
2. Using CacheInterface::get instead of CacheItemPoolInterface makes hit rate better but not ideal and still got small % of errors during load testing
3. The only working variant we had giving us 100% rate looks quite strange but anyway

        $cacheKey = rawurlencode($cacheKey);

        $item = $this->cache->getItem($cacheKey);
        $raceConditionHit = $item->isHit() && !\is_array($item->get());

        if (($this->debug && ! $this->refresh($cacheKey, $class)) || ! $item->isHit() || $raceConditionHit) {
            if ($raceConditionHit) {
                usleep(100);
            }
            $this->cache->save($item->set($this->delegate->{$method}($reflector)));
        }

        return $item->get();

We understand that problem is quite unique and the package is not actively maintained, but since this class is final, it is hard to override this code and it would be great to have working solution in one of the next releases.

Thank you

[stof]

Which PSR cache implementation are you using ?

This behavior looks weird to me. AFAICT, the only way $item->get() would return null when isHit is true in a spec compliant cache implementation is if null was saved in the cache explicitly. And this PsrCachedReader won't save null in the cache.

[dsyrvachov]

It's Symfony\Component\Cache\Adapter\PhpArrayAdapter
"symfony/cache": "v5.4.3"

[inri13666]

@stof I think it does not really depend on the real Cache Provider used because this method except to return only array, but should support null as well
https://github.com/doctrine/annotations/blob/1.13.x/lib/Doctrine/Common/Annotations/PsrCachedReader.php#L150

Please take a look at the public interface from Psr\Cache package
https://github.com/php-fig/cache/blob/master/src/CacheItemInterface.php#L46-L49
and it defines null as the possible return value

 @return mixed
     *   The value corresponding to this cache item's key, or NULL if not found.

It seems the method signature should be changed to

    /** @return mixed[]|null */
    private function fetchFromCache(
        string $cacheKey,
        ReflectionClass $class,
        string $method,
        Reflector $reflector
    ): ?array {

Or the method should return an empty array for example

return $item->get() ?? [];

[stof]

Well, that description is the whole point. It returns null only if it is not found (assuming the cached value is not nullable). And isHit() indicates whether the item is found.

[inri13666]

Yes, the point is in the description, so the fetchFromCache should also somehow handle this case to avoid such errors :)

[derrabus]

No, it shouldn't. If they occur, the cache implementation is broken and should be fixed instead. If you can reproduce this issue with Symfony Cache, you should report it to Symfony.

---

That being said, I'd really recommend to not evaluate annotations on a hot path or in high-load scenarios, even with a cached reader. Cache the information you extract from the annotations, not the annotations themselves.

[dxops]

I had a similar case while ago using symfony ACL cache symfony/security-acl#47

It happens while using Redis with eviction policy, so key got removed in moments between isHit and get calls. You need to check get result is not null or combine isHit and get into a single Redis request.

[stof]

isHit and get are not doing separate calls to Redis in a spec-compliant cache implementation. To respect the spec, the call to Redis has to be made in getItem instead.