wp-memcached icon indicating copy to clipboard operation
wp-memcached copied to clipboard

Undefined index notice when stats get reset

Open kevinfodness opened this issue 5 years ago • 3 comments

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

  • WordPress core's PHPUnit abstract testcase, in the flush_cache method: https://github.com/WordPress/wordpress-develop/blob/master/tests/phpunit/includes/abstract-testcase.php#L343
  • WP-CLI, in the wp_clear_object_cache method: https://github.com/wp-cli/wp-cli/blob/5dc1c5835dcf532dd8530b83c32a2e249211df94/php/utils-wp.php#L300

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 avatar May 07 '20 19:05 kevinfodness

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.

kevinfodness avatar May 07 '20 19:05 kevinfodness

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..

dd32 avatar Jun 10 '20 03:06 dd32

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.

kevinfodness avatar Jun 12 '20 19:06 kevinfodness