Warning: array_key_exists() expects parameter 2 to be array, null given

[tobiasmuehl]

PHP version: 7.3

Description

Getting these warnings reported in Sentry on some requests. The chance of this error happening is roughly 1%.

https://github.com/php-http/cache-plugin/blob/master/src/CachePlugin.php#L150

How to reproduce
Not entirely sure to be honest. Using the PSR16 adapter of https://www.phpfastcache.com/ with the Redis driver, and the Symfony cache adapter to convert PSR16 to PSR6. It only happens on a small subset of requests. I'm wondering if this is a race condition, when the cache item expires in between cache lookups. First call sees if item is in cache, by the time the second call tries to get the data, the item has expired.

Possible Solution

Check $data for null before processing further.

[dbu]

hm, this sounds really weird. does PSR specify whether CacheItem::get is to check expiration again?
but even then, 1% sounds like way too much for race conditions, unless you are dealing with cache lifetimes in the range of 1 single second.

this smells like some oddity of the cache implementation or of how the adapter works.

i guess if we would run phpstan on this, it would complain that $data may be null. therefore it seems not entirely wrong to me to add null !== $data as a first check in the if condition. before we do that though: can you please check if there is any possiblity that we could be setting the cache key to the value null in some situations? i guess that would be legal for the PSR cache specs, and could also lead to the situation you observe.

[tobiasmuehl]

Using the default key generator (SimpleGenerator), so cache key can't be null.

1% sounds like way too much for race conditions, unless you are dealing with cache lifetimes in the range of 1 single second.

It's a high traffic application with millions of requests per day. Do you think the TTL is related to the occurrence rate of a possible race condition? I'd think it doesn't matter how long a cache entry is valid - the race condition appears very close to cache expiration, whether that's in 1 second from now or 1 hour from now.

[dbu]

TTL

the thing is: if you cache a page for 1 hour, what is the probability that it is only requested 99 times, and then request 100 falls into the same millisecond where the cache expires. even if somehow it would take a whole second because the isHit and getData, if you had even distribution of your requests, you could expect 1 out of 3600 requests to fail.

the only way i could imagine how you get to 1% by chance would be if this is something about a cronjob that runs at the same frequency as the cache TTL.

null

the cache key is not null, the problem is that the $data is null. what i meant is whether there is any way we could be setting the data at $key to null instead of having actual data. looking at https://github.com/php-http/cache-plugin/blob/master/src/CachePlugin.php#L200 even if the response would somehow be null, we still set the expiresAt key. and the other place where we update whats at the cache is https://github.com/php-http/cache-plugin/blob/master/src/CachePlugin.php#L182 - data also can't be null here.

so i suspect that something else in your system might sometimes overwrite the data at the cache key. or that the cache implementation can lose data but still report isHit, which would seem really strange.

[tobiasmuehl]

something else in your system

quite possible since I'm running an esoteric caching stack. I've forked this repo and added the null check, for me personally this is an adequate solution

[dbu]

hm, i added phpstan up to level 8 in #74 and phpstan does not complain.

not sure if there are special rules for the use case though. as theoretically, anything could put a value into the cache, we should probably not rely on the cached value to be the array we expect it to be. i added a change to check (not for null but for is_array)

[tobiasmuehl]

Okay, I will try running the phpstan branch and let you know if this fixes my issue