markjaquith / WP-TLC-Transients

GNU General Public License v2.0
341 stars 37 forks source link

memcached object cache not updating when using `background_only()` #29

Open roborourke opened 10 years ago

roborourke commented 10 years ago

I've got a problem with using an object cache and updates on background. It doesn't seem to ever update the object cache even though it definitely runs the update function.

To reproduce this install the w3 total cache plugin and activate the object cache using memcached. It may happen with other object cache dropins too but I've not tried it yet.

If I don't use background updates then it works without a problem. Any suggestions or workarounds would be good, I'll keep looking into the cause in the meantime.

Rarst commented 10 years ago

Do try different Object Cache backend. There had recently been reports about object caching implementation currently broken in W3TC.

roborourke commented 10 years ago

Same result using the memcached dropin :( https://wordpress.org/plugins/memcached/

roborourke commented 10 years ago

Ok this is memcached specific. Maximum expiry time is 30 days so the far future expiry of 1 year causes it to just return nothing false every time.

I'll make a pull request but would like to discuss options first - whether we try to detect if memcached is in use and use a max() check or we make the year thing a variable so developers can change it if needed. I think it should be in the core of TLC though as plugins that use it can't anticipate the environment they're being installed in.

Rarst commented 10 years ago

As far as I remember Object Cache in WP doesn't really expose (as in proper API) the type of backend it implements. So it is problematic to detect with confidence.

I'd say options are:

  1. Do nothing, document necessity to deal with expiration time on either side.
  2. Default expiration time to 30 days when memcached is detected (regardless if it's actually used as backend since no reliable way to tell).
  3. Default expiration time to 30 days in general, instead of one year.
roborourke commented 10 years ago

Option 3. definitely easiest & neatest to implement. I'd argue a year isn't necessary. Option 2. easy enough but like you say, may not actually be getting used

Option 4 would be to add a method to set the persistent expiry as part of the chained call eg:

tlc_transient( 'key' )
  ->expires( 300 )
  ->background_only()
  ->updates_with( 'update' )
  ->persist_for( 30*24*60*60 )
  ->get();

What gets your vote? I prefer 3 but 2/4 both ok.

Rarst commented 10 years ago

Separate expiration and persistence in 4 is tad confusing/excessive I think.

Personally I think that 3 would make it "just work" properly for more people (who otherwise would get bitten by memcached), than it would harm people with use cases for long expiration (who would need to adjust configuration).

roborourke commented 10 years ago

Yep. I'll make a pull request to that effect then, cheers :)

tollmanz commented 10 years ago

Possibly related: https://github.com/tollmanz/wordpress-memcached-backend/issues/35. Should this be addressed at the object cache level?

Rarst commented 10 years ago

Specific implementations might want to handle, but no way to make sure every existing and hypothetical implementation out there deals with it, so still needs to be addressed (maybe) here as well.

tollmanz commented 10 years ago

That makes sense. I'm also just trying to get a handle on the bug as the OP for the ticket I linked left me hanging. If there is a reason to handled it at the OC layer, I'd be more than happy to get it implemented.

roborourke commented 10 years ago

@tollmanz the main reason is that passing an expiry greater than 30 days to memcached fails so nothing gets cached and it returns false, it's worth doing the extra check on the OC layer if you know you're using memcached.

In the case of TLC the update function is being run nearly every time but even if it gets a result the OC returns false due to 30+ day expiry time.

mbijon commented 10 years ago

The problem is specific to memcache because of how it interprets expiration values: "Expiration times can be set from 0, meaning "never expire", to 30 days. Any time higher than 30 days is interpreted as a unix timestamp date." (From: https://code.google.com/p/memcached/wiki/NewProgramming#Expiration)

The best-case here would be for all memcache object caches to match this expiration interpretation. But, since that's a long road, it might be best if TLC Transients used time()+1yr as the long-expire value. Then it would be interpreted correctly by memcache while just being an exceedingly long timeout on other caching backends.

Rarst commented 10 years ago

I think this shouldn't be catered to memcached specifics that far. The point of Transients API is to be abstracted from back end. How do we know that timestamp-like expiration won't break in some other backend, etc?

It's probably job of Object Cache to transparently interpret >30d expiration into timestamps, but TLC Transients is wrong place for handling it in my opinion.