WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 19 months ago

Last modified 11 months ago

#22661 closed defect (bug) (fixed)

Allow object caches to degrade gracefully

Reported by: markjaquith Owned by: markjaquith
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Cache API Keywords:
Focuses: Cc:

Description

Because of the way object caches are loaded, if a custom object cache can't run (say, Memcached or APC is not actually installed), it cannot gracefully degrade to the built-in object cache.

Witness this code in wp_start_object_cache():

 	if ( ! function_exists( 'wp_cache_init' ) ) {
		if ( file_exists( WP_CONTENT_DIR . '/object-cache.php' ) ) {
			require_once ( WP_CONTENT_DIR . '/object-cache.php' );
			$_wp_using_ext_object_cache = true;
		} else {
			require_once ( ABSPATH . WPINC . '/cache.php' );
			$_wp_using_ext_object_cache = false;
		}
		$first_init = true;
	} else if ( !$_wp_using_ext_object_cache && file_exists( WP_CONTENT_DIR . '/object-cache.php' ) ) {
		// Sometimes advanced-cache.php can load object-cache.php before it is loaded here.
		// This breaks the function_exists check above and can result in $_wp_using_ext_object_cache
		// being set incorrectly. Double check if an external cache exists.
		$_wp_using_ext_object_cache = true;
	}

So a custom object cache is loaded. If it wants to bail and defer to the built in object caching, it can do that by doing the include itself. But then WordPress sets $_wp_using_ext_object_cache = true; after that require. So WordPress thinks it is using an external object cache when it's actually not. This leads to oddness.

This can sometimes be hacked around by adding a callback to the very first WP action available that sets $_wp_using_ext_object_cache = false;, but that has issues: calls to the object cache might be made before that code can run, and add_action() is not always available at that point (because some advanced-cache.php drop-ins load the object cache really early. See Batcache.

Proposed solution: change the order of the require_once() and the setting of $_wp_using_ext_object_cache. That way, the external object cache can override the variable.

Attachments (5)

22661.patch (598 bytes) - added by SergeyBiryukov 6 years ago.
22661.2.patch (751 bytes) - added by SergeyBiryukov 5 years ago.
22661-cache_via_filters.diff (5.0 KB) - added by jipmoors 5 years ago.
implemented filters which enable cache fall through
22661-cache_via_filters-ext_object_filter.diff (5.2 KB) - added by jipmoors 5 years ago.
cache via filters + filter to enable external object cache
22661-logic-refactor.diff (3.0 KB) - added by markjaquith 19 months ago.

Download all attachments as: .zip

Change History (31)

#1 @wonderboymusic
7 years ago

this is mildly related: #21401

#2 @scribu
7 years ago

  • Cc scribu added

#3 @aaroncampbell
7 years ago

  • Cc aaroncampbell added

#4 follow-up: @wonderboymusic
6 years ago

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

Unless I'm missing something, we did this here: [25289].

#5 @SergeyBiryukov
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.9
  • Resolution fixed deleted
  • Status changed from closed to reopened

We still call wp_using_ext_object_cache( true ) after the require_once(), so this ticket was not addressed in [25289]. I guess 22661.patch should resolve this.

#6 @SergeyBiryukov
5 years ago

Looks like 22661.patch might not work after [27634] (it was relying upon the initial null value).

22661.2.patch is another take. A custom object cache should be able to bail and defer to the built-in caching by including the file and calling wp_using_ext_object_cache( false ).

Last edited 5 years ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.


5 years ago

#8 @nacin
5 years ago

[27634] can be reverted if necessary, but it felt like proper behavior to me. 22661.2.patch sounds good to me.

#9 @nacin
5 years ago

Well, there is some additional code here:

	} else if ( ! wp_using_ext_object_cache() && file_exists( WP_CONTENT_DIR . '/object-cache.php' ) ) {
		// Sometimes advanced-cache.php can load object-cache.php before it is loaded here.
		// This breaks the function_exists check above and can result in $_wp_using_ext_object_cache
		// being set incorrectly. Double check if an external cache exists.
		wp_using_ext_object_cache( true );
	}

So this wouldn't work in the case of Batcache loading object-cache.php earlier.

#10 @nacin
5 years ago

  • Milestone changed from 3.9 to Future Release

I'm going to punt this until it has an approach that handles all cases.

#11 @jipmoors
5 years ago

  • Keywords 2nd-opinion added

A solution would be to set a reserved key (_wp_cache_active?) and retrieve it to see if the cache is actually active before setting the wp_using_ext_object_cache blindly to enabled.

As a counter argument I realise that you do create overhead, but cache should be fast and lean in it's nature.

I guess the problem with an external cache provider that is not working, is that the functions would be defined in the implementation of that external cache. Which means that all cache should be disabled and we can't revert to core cache.php.

Last edited 5 years ago by jipmoors (previous) (diff)

#12 @jipmoors
5 years ago

  • Keywords has-patch removed

Another idea is to always include the cache.php from core and add filters to the public functions.
This way the functions act as a permanent gateway into the custom cache implementations, which would give us more security and testing abilities.

Also this solution would supply a clean way for the cache to have one or more fallbacks, which should give cache plugin creators more freedom and cleaner code.

So far the only problem with this approach is that wp_cache_get has the 4th parameter &$found, which a null check would provide for a correct response.

#13 @mpvanwinkle77
5 years ago

As far as design goes, I generally agree with @jipmoors that, given the WordPress API, any class "overloading" should take place in the context of a filter and NOT simply placing the file in a certain location.This change would solve both this issue and the rather clumsy way that external object caches are currently installed which can be problematic on locked-down filesystems. This comment isn't intended to block this patch but only to suggest a ticket for overhauling this in the future. Obviously the implementation would have to be to respect the file-based overloading first so as not to create any backward compatibility issue. But over time that approach could be deprecated.

#14 @jipmoors
5 years ago

If you use an object-cache.php (or advanced-cache.php) without the wp_cache_init function then the wp_using_ext_object_cache would remain false and you can attach to the filters and have the core cache.php being loaded. Which in turn calls the apply_filters.

So without any modifications in file loading you can move forward by just enabling the apply_filters in all the cache.php functions.

The only note with this is that first_init would be called every time. Which is actually a good thing, because we need the WP_Object_Cache to be instantized to provide fallback functionality.

I see some ways of combating this problem:

  1. (favorable) Check for a define or function to verify a filter implementation is present
  2. Using a new file to check against for cache implementation via filters

Adding to that the notion of writing unit-tests on the public functions of the Cache API instead of the class would make even more sense.

@jipmoors
5 years ago

implemented filters which enable cache fall through

@jipmoors
5 years ago

cache via filters + filter to enable external object cache

#15 @jipmoors
5 years ago

Using filters it not going to work. The only problem is wp_cache_get which needs to be able to return a 'null' when the cache contains this value for a key.

A method that allows you to registers a cache handler with a priority would be a solution.
The public functions will work through the registered handlers until the request is completed.
The handlers should extend the WP_CacheHandler abstract class to ensure minimal function compliance.

I think this is a concept that has a lot of potential and will see if I can make an elegant implementation.

Please forget/ignore/remove my previous patches.

#16 follow-up: @jipmoors
5 years ago

  • Keywords 2nd-opinion removed
  • reverted to silence -
Last edited 5 years ago by jipmoors (previous) (diff)

#17 in reply to: ↑ 16 @SergeyBiryukov
5 years ago

Replying to jipmoors:

The code referenced to is no longer actual.

There has been a modification to check for the existence of wp_cache_init after including the object-cache.php which allows for core cache usage. (r25289)

Right, but [25289] did not resolve this ticket, see comment:5.

#18 @jipmoors
5 years ago

  • Keywords 2nd-opinion added

#19 @jipmoors
5 years ago

When object-cache.php has been loaded and wp_cache_init exists but wp_using_ext_object_cache(false) is used to disable external cache from within this code. Then wp_cache_init is already defined.

How is cache.php going to be loaded when it depends on it's own implementation of that function.

#21 in reply to: ↑ 4 ; follow-up: @markjaquith
19 months ago

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

Replying to wonderboymusic:

Unless I'm missing something, we did this here: [25289].

You are right.

If your object-cache.php does not define the wp_cache_init() function, wp_using_ext_object_cache( true ); will not be called, and this will run, loading WP's in-memory cache:

	if ( ! wp_using_ext_object_cache() ) {
		require_once ( ABSPATH . WPINC . '/cache.php' );
	}

So you can just wrap your object-cache.php content (including all the function definitions) in a prerequisites check. Like:

if ( class_exists( 'Redis' ) ) {
    // Original file contents here.
}

Or, better, move the object cache file you're using to redis-object-cache.php and do:

if ( class_exists( 'Redis' ) ) {
   include( __DIR__ . '/redis-object-cache.php' );
}

So you can keep the original file unadulterated.

Closing this, as the original intent ("allow custom object caches to gracefully degrade") is possible now.

#22 in reply to: ↑ 21 @jtsternberg
19 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to markjaquith:

If your object-cache.php does not define the wp_cache_init() function, wp_using_ext_object_cache( true ); will not be called, and this will run, loading WP's in-memory cache:

I’m still seeing issues with this approach, but only on multisite. Because ms-settings.php calls wp_start_object_cache() again, it makes WP think an external object-cache exists: http://b.ustin.co/zc3syn

So it means WP did load it's default implementation (wp-includes/cache.php), which is good as some cache will be used some times, but $_wp_using_ext_object_cache is set to true, which is bad because wp_using_ext_object_cache() is used quite a bit to determine if some things will attempt to be cached to a persistent object cache (causing extra queries), and to determine if some things don't need to be cached (causing extra queries). So something like this https://github.com/jtsternberg/object-cache.php-replacement/blob/master/object-cache.php is needed to reset that global during the first available hook.

It might be a decent solution for wp_using_ext_object_cache() to have a filter, as I think you could then override it for this scenario.

OR, allow the object-cache.php file to define define( 'USING_EXT_OBJECT_CACHE', false ); which could override any logic that occurs downstream.
Then wp_using_ext_object_cache would look something like:

function wp_using_ext_object_cache( $using = null ) {
	global $_wp_using_ext_object_cache;
	$current_using = $_wp_using_ext_object_cache;

	if ( defined( 'USING_EXT_OBJECT_CACHE' ) ) {
		$using = USING_EXT_OBJECT_CACHE;
	}

	if ( null !== $using ) {
		$_wp_using_ext_object_cache = $using;
	}
	return $current_using;
}

#23 @markjaquith
19 months ago

  • Keywords needs-testing added

Replying to jtsternberg:

I’m still seeing issues with this approach, but only on multisite.

I conferred with @jtsternberg a bit on this, and yeah, that "else if" block gets triggered on subsequent runs of the function (which happens in multisite), which causes it to think an external cache is being used when it is not.

We worked on a refactor, and came up with 22661-logic-refactor.diff, which converts $first_init to a static, which enables us to tell if it is a first run of the function (during which we should try to load object-cache.php or detect that advanced-cache.php has already loaded it) or a secondary run of the function (during which we should do neither).

@jtsternberg tested with Batcache, multisite, and with an object-cache.php that does not define wp_cache_init() (our "fall back to normal in-memory caching" scenario).

#24 @markjaquith
19 months ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from reopened to closed

In 42723:

Cache API: Allow external object caches to gracefully degrade to the default object cache.

Rework logic for how external object caches are detected, so that if
an external cache does not define a wp_cache_init(), the built-in
object cache will be used.

Object caches can now wrap their entire contents in logic checks. So a
Redis caching backend could make sure that the Redis PHP class is
available before defining all the caching functions. And if Redis is
not available, the site doesn't break or throw errors or think it is
using caching when it isn't. This is particularly useful for doing
local development, where you might want to develop on a site without
running Memcache or Redis like you are in production.

  • Accounts for multisite, which may re-initialize the object cache

multiple times.

  • Accounts for object caches that may include object-cache.php during

advanced-cache.php (before WP loads it).

Props jtsternberg, markjaquith.
Fixes #22661.

Last edited 19 months ago by markjaquith (previous) (diff)

#25 @markjaquith
19 months ago

  • Keywords 2nd-opinion needs-testing removed
  • Milestone changed from Future Release to 5.0

#26 @johnbillion
11 months ago

  • Milestone changed from 5.0 to 5.1
Note: See TracTickets for help on using tickets.