Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add faux-flush support #49

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

webaware
Copy link

When multiple websites share a single Memcached instance, it's bad form to allow one site to flush the entire Memcached cache. Also, flush_all can be disabled to prevent just this situation.

Better an empty room, than a bad tenant!

This PR adds faux-flush support through a generated salt that is replaced each time wp_cache_flush() is called. The delay parameter is ignored, and all items for the current website config are essentially "forgotten" because they can no longer be retrieved. They'll eventually be evicted either due to expiry or for memory reclamation based on LRU.

Faux-flush is enabled by defining the constant WP_CACHE_FAUX_FLUSH as truthy. This allows the drop-in to continue functioning as it currently does, unless specifically configured to use faux-flush.

An admin menu item is added to the Tools menu, allowing admins to request a faux flush at any time. As a developer, this makes my life much easier :) and can resolve problems that might otherwise require either a support request to the website host, or editing wp-config.php to change WP_CACHE_KEY_SALT (what I've been doing up until now).

@webaware
Copy link
Author

Just fixed a bug on translation of the admin messages, and restricted access to faux-flush in the admin on multisite to super admins.

@tollmanz
Copy link
Owner

Hey @webaware! Thank you so much for all of the activity on this project. It's always really exciting to have people contributing!

I just wanted to pop in and let you know that I'll be taking a close look at your work next week. I'm currently on vacation and am AFK. I wanted you to know I'm very thankful for the help and I'll be giving it a lot of thought next week.

@webaware
Copy link
Author

No worries @tollmanz, I wasn't expecting an immediate response or anything. I just went back to the code to add that multisite super admin restriction and noticed the l10n bug. I'm good at this end, it's all working well. The faux-flush was something I needed for a server with multiple sites including several under active development.

Thanks for sharing your work on the drop-in -- it's easily the best Memcached object caching implementation out there, and your excellent code comments have made it really easy to understand. I hope to contribute more once I get some clear air, maybe next year.

@tollmanz
Copy link
Owner

Hey @webaware!

I'm finally get around to taking a look at this. You bring up a really valid concern. I'm not sure what the right answer is at the moment, but I generally like what you are doing.

A couple early thoughts:

  • I do not want the admin stuff in this plugin. I may decide to add another plugin that adds more admin features in the future, but right now, I want this to be mostly UI free. I have another plugin that allows for attaching cache refresh functions to the admin bar that is make for situations like these.
  • I really like the cache versioning/incrementor method that you are using here. I think the best way to update the version is to use time(). I think it might be a cleaner method that what you are doing. I've written about how to do this.

I need to really think about this one and may solicit more opinions as this is a pretty big change. I do agree that allowing one site to flush the full cache can be a big issue, but is this a configuration or software issue? In other words, should you allow your sites to share a memcached instance? Perhaps each site should use a different memcached instance. I think that this becomes an issue though when you are using Multisite. You probably don't want to spin up a new memcached instance for each MS site.

@webaware
Copy link
Author

I do not want the admin stuff in this plugin. I may decide to add another plugin that adds more admin features in the future, but right now, I want this to be mostly UI free. I have another plugin that allows for attaching cache refresh functions to the admin bar that is make for situations like these.

Fair enough, I'll move that into a separate plugin for my own use and remove from this PR. Will check out the other plugin, it might satisfy my needs here (although a button on the admin bar could be too inviting for some people to hit!)

I really like the cache versioning/incrementor method that you are using here. I think the best way to update the version is to use time(). I think it might be a cleaner method that what you are doing. I've written about how to do this.

time() is not sufficient, it can easily lead to collisions when incrementing in the same second (can lead to stale cached data). You want microtime() here at least, which still doesn't guarantee anything, but is better. uniqid() is basically microtime() with a more succinct format. I should probably be calling it with the $more_entropy argument set to more properly avoid collisions.

I do agree that allowing one site to flush the full cache can be a big issue, but is this a configuration or software issue? In other words, should you allow your sites to share a memcached instance? Perhaps each site should use a different memcached instance.

Essentially true, but not always possible when dealing with hosting companies. I wrote this PR because I have a client with a managed VPS hosting over 100 sites (most tiny) and one memcached instance. It was enough trouble getting that established. It also didn't honour calls to wp_cache_flush() so either the Memcached extension isn't passing on flushes or it's disabled in memcached.

I think that this becomes an issue though when you are using Multisite. You probably don't want to spin up a new memcached instance for each MS site.

s/don't want/can't/ probably. This PR doesn't do anything differently there either, and nor should it I reckon. When flushing the cache, you'd expect user meta to be flushed, and that's cross-blog in multisite, and blog data can be associated with users, so... flushing cache in MS should flush for all blogs and the site. Perhaps each site/network in a multi-site (multi-network) blog could have its own faux-flush key; don't know if that would be safe though, would need investigation.

When I get a chance (ha!) I'll move that admin code out and bump this PR.

@spacedmonkey
Copy link

I agree with @tollmanz admin functionality has no place in a drop-in.

I like the idea of not flushing the cache, but invalidating the salt is a great idea. It is like flushing, but not really flushing it. The problem is that, you want sites to be able to call the flush command to clear the cache, however, you don't want one site on the network to clear the whole cache. It if you have say 500 sites in the network, anyone of them could flush the cache at anytime, meaning the cache could always be empty. So if you are sending the flush command, only the site that sent it should have it's cache cleared.

So we should have two salts. A global salt faux for cached items in the global scope, like users and site meta and a local salt faux. This local salt, would be linked to your blog id. This means that local salts should be loaded dynamically in a method and will accessed via the blog id. When a flush command is sent, the global and local salt should be changed. The current site's local salt should be changed, meaning that other local salt remain valid and objects are not cleared.

@tollmanz
Copy link
Owner

time() is not sufficient, it can easily lead to collisions when incrementing in the same second (can lead to stale cached data). You want microtime() here at least, which still doesn't guarantee anything, but is better. uniqid() is basically microtime() with a more succinct format. I should probably be calling it with the $more_entropy argument set to more properly avoid collisions.

Fair enough...we should eventually dive in and figure out the best method here. FWIW, I've had good success with time() as an incrementor given that it would be nearly unheard of to have race conditions with a cache flush. That said, I'm all for trying to tackle the weird edge cases, so we should definitely try hard to handle this the best we can.


Hi @spacedmonkey! Thanks for stopping by and weighing in on the issue. I definitely see your point.

The problem is that, you want sites to be able to call the flush command to clear the cache, however, you don't want one site on the network to clear the whole cache.

Yes...exactly. To give you some history, when I initially created this plugin, I was creating it as an alternative to the original WordPress Memcached Object Cache. This was developed by Automattic for running WordPress.com, which is an enormous WordPress MultiSite installation. The wp_cache_flush() function in that library does not flush the cache (for very good reason). When I started working with object caching, I had a need for flushing the cache and thus made sure that the library would flush the cache.

Through this conversation (and similar ones offline), I very much see the need for more flexibility for cache flushing. As a result, I'd like to build the following into the library:

  1. Configuration for blocking all cache flushing. This would be useful for admins who know that such action would be catastrophic.
  2. Add site based cache flushing. This would allow one site to be flushed without flushing the cache for another site. This would be based on the cache version I discussed earlier in the thread.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7391c0a on webaware:simulated-flush into * on tollmanz:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants