Automattic / wp-memcached

Memcached Object Cache for WordPress.
https://wordpress.org/plugins/memcached/
GNU General Public License v2.0
158 stars 55 forks source link

Undefined index notice when stats get reset #50

Open kevinfodness opened 4 years ago

kevinfodness commented 4 years ago

Various systems reset the stats array to an empty array, which deletes all of the keys, such as 'get' and 'add'. Common examples include:

This was recently fixed in the VIP WP CLI helper: https://github.com/Automattic/vip-go-mu-plugins/commit/515b71d193535b18b9b275e6c44a458c4e1ffca6

Since the property is public and we can't control for every use case, one thing we can do is to ensure that the increments are happening safely, rather than assuming the property exists, as is done here: https://github.com/Automattic/wp-memcached/blob/master/object-cache.php#L319

kevinfodness commented 4 years ago

I submitted a PR with a light-touch fix in #51. There is still a potential issue here, in that cache hits and cache misses are assigned by reference to sub-elements in that array: https://github.com/Automattic/wp-memcached/blob/master/object-cache.php#L655-L656

A more robust solution could use magic __get and __set methods and make the stats property private, along with cache_hits and cache_misses, and allow the class to be in full control over getting, setting, and resetting those values to ensure that the keys all exist. However, the solution I proposed in #51 does not introduce major changes to the plugin architecture, so it would be safer to integrate near-term, and it doesn't make the problem worse or introduce any other side effects.

dd32 commented 4 years ago

I'm wondering if a better method wouldn't be to just have Core/WP-CLI unset the global and call wp_cache_init() again instead to avoid just this. For Memcache specifically that would mean re-opening connections though.

Perhaps another option is to suggest a garbage_collection-style method that core/cli can call to free up memory/clear the local cache without affecting everything else. Such a function would definitely be useful to me..

kevinfodness commented 4 years ago

I agree that the issue should be fixed more holistically, both in other libraries that use this plugin, as well as more gracefully within the plugin itself. However, the fact that the stats array can be reset because it's a public property justifies some look-before-you-leap code within this plugin as well.