WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#11431 closed enhancement (fixed)

object cache's terms bucket potentially contains junk

Reported by: Denis-de-Bernardy Owned by: ryan
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Cache API Keywords: early has-patch commit
Focuses: Cc:

Description (last modified by Denis-de-Bernardy)

We've a few occurrences of things such as:

wp_cache_add( $cache_key, $terms, 'terms' );

... in taxonomy.php. These are not flushed when the terms cache gets flushed, and end up stored in memcached indefinitely. We should at least add a timeout, maybe after a day.

Better yet, we should flush them properly.

Attachments (1)

11431.diff (721 bytes) - added by Denis-de-Bernardy 4 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Denis-de-Bernardy4 years ago

  • Keywords has-patch added

see also: r9102, r8225, etc.

Denis-de-Bernardy4 years ago

comment:2 Denis-de-Bernardy4 years ago

there potentially is a better way to do this. such as is done in get_pages():

	$key = md5( serialize( compact(array_keys($defaults)) ) );
	if ( $cache = wp_cache_get( 'get_pages', 'posts' ) ) {
		if ( is_array($cache) && isset( $cache[ $key ] ) ) {
			$pages = apply_filters('get_pages', $cache[ $key ], $r );
			return $pages;
		}
	}

comment:3 filosofo4 years ago

I don't think the get_pages() approach scales very well. I've noticed several times that sites with a large number of pages at points exhaust the available memory in one of the cache methods. Since for a typical WP site there are many more terms than pages, I'd expect this to crop up more often if used for terms.

Why not do something like I suggested a while ago: #4476. Delete cache items by group name. Then the clean_term_cache() could just flush all members of the "terms" group. Problem solved, right?

comment:4 ryan4 years ago

The last_changed method used by terms was done to avoid the problems of the pages style caching where the arrays could get huge. Currently, the last_changed method relies on old cache keys eventually being pushed out of the cache. Using a timeout to reclaim them faster seems fine.

I've yet to see flush by group (or even just flush) implemented solidly for all of the various cache backends for both WP and WPMU.

comment:5 Denis-de-Bernardy4 years ago

Replying to filosofo:

Why not do something like I suggested a while ago: #4476. Delete cache items by group name. Then the clean_term_cache() could just flush all members of the "terms" group. Problem solved, right?

It would, if memcached allowed to partially flush keys using wildcards. Trouble is, memcached offers only two option, mark this very specific key as obsolete, and mark all keys as obsolete.

there are a few bits in backpress' memcached that allow to to flush an entire bucket, but I'm 100% sure that it is not concurrent proof: (I interacted the pecl package's author last year in order to convince him to implement locks for memcached-based session handling. It's the only thing in the memcached pecl with any type of locking.)

@Ryan: the file I sent you the other day implements flush (as in, full flush) quite fine. But no partial flush.

comment:6 Denis-de-Bernardy4 years ago

  • Description modified (diff)
  • Keywords dev-feedback added

I've been working with the patch applied for a couple days. No side effects apart from the obvious extra query per day.

@Ryan: do you need it massaged any further?

comment:7 hakre4 years ago

I'd like to rate this as tested. Denis can you report that it made another 12 days working properly?

comment:8 Denis-de-Bernardy4 years ago

  • Keywords tested added; dev-feedback removed

I'm afraid I can't, since I svn switched back to the 2.9 branch upon seeing a few scary looking changes creeping in. But definitely tested, yes.

comment:9 Denis-de-Bernardy4 years ago

  • Description modified (diff)
  • Keywords commit added

comment:10 nacin4 years ago

  • Keywords early added; tested removed
  • Milestone changed from 3.0 to 3.1

comment:11 shidouhikari4 years ago

  • Cc shidouhikari added

But memcached doesn't flush stuff after some time?

I use WP File Cache plugin and by default it gets filemtime to know when cache was created, and if it's older than 1h cache is deleted and returns false. I changed default to 1 day and works fine.

It's really scary having data eternally cached by default. Even more when wp_cache_add() only adds data to cache if it's not already cached, and wp_cache_replace() also only replace if data is already cached.

A bug in any data editing code that doesn't delete cache may make old data be used forever.

comment:12 Denis-de-Bernardy4 years ago

But memcached doesn't flush stuff after some time?

Not really... It kicks old stuff out of the cache automatically when it runs out of memory. But there's no timeout per se if you don't pass one.

Related: #14713

comment:13 markjaquith4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [15583]) Do not cache terms indefinitely... use TTL of one day. fixes #11431. props Denis-de-Bernardy

comment:14 nacin4 years ago

  • Milestone changed from Awaiting Triage to 3.1
Note: See TracTickets for help on using tickets.