Make WordPress Core

Opened 9 years ago

Last modified 21 months ago

#30430 new defect (bug)

WP_Object_Cache does not properly cache objects nested in arrays

Reported by: jamesgol's profile jamesgol Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.0
Component: Cache API Keywords: reporter-feedback has-patch needs-testing needs-unit-tests needs-refresh
Focuses: Cc:

Description

I noticed on a plugin that cached data wasn't coming out the way it was inserted. What was happening is that it was an array of objects. The code would then go and change the original data and the data in the cache is changed. I would expect data retrieved from the cache to be exactly the same as what was inserted.

To make sure there was minimal performance impact I did some tests and the test script is attached. Initially I thought unserialize(serialize($data)) would be a good route, but it was twice as slow as rolling your own solution. I tried numerous methods and the function I initially build was always the fastest.

The transient/option table doesn't have this problem because it calls maybe_serialize.

Another way to fix this would be to call maybe_serialize on the set, and maybe_unserialize on a get but that would spread the cost to both functions and I think it is better to have the set be slightly more expensive than having every set and get take slightly longer.

Attachments (8)

wp-deep-copy.diff (1.1 KB) - added by jamesgol 9 years ago.
Patch to resolve issue
test.php (2.9 KB) - added by jamesgol 9 years ago.
Code to test efficiency of new function
testing.txt (1.9 KB) - added by jamesgol 9 years ago.
Output of my test code to show efficiency of functionn
30430.unit-tests.diff (2.6 KB) - added by valendesigns 9 years ago.
bug30430.php (534 bytes) - added by jamesgol 9 years ago.
Demo of bug using shortcode
wp-deep-copy-2.diff (1004 bytes) - added by jipmoors 9 years ago.
optimization: removing duplicate functionality
wp-deep-copy-withclass.diff (1.1 KB) - added by jamesgol 21 months ago.
Updated patch with changes from 2/2020
wp-deep-copy-withclass2.diff (1.1 KB) - added by jamesgol 21 months ago.
Bugfix

Download all attachments as: .zip

Change History (21)

@jamesgol
9 years ago

Patch to resolve issue

@jamesgol
9 years ago

Code to test efficiency of new function

@jamesgol
9 years ago

Output of my test code to show efficiency of functionn

#1 @johnbillion
9 years ago

  • Keywords reporter-feedback has-patch needs-testing needs-unit-tests added
  • Version changed from trunk to 2.0

Thanks for the report. Can you provide some example code which demonstrates the issue please?

#2 @valendesigns
9 years ago

  • Keywords needs-unit-tests removed

@jamesgol I turned your test.php into unit tests and they all pass, so I'm not sure what the issue is when the data going in is the same as the data coming out. Please provide more information on how you got it to break.

Cheers,
Derek

#3 follow-up: @dd32
9 years ago

To trigger this, I'm fairly certain it would need to be meshing with an external object cache, and have the local cache within PHP (The global array) cleared between set/fetch (ie. a different pageload).

#4 in reply to: ↑ 3 @valendesigns
9 years ago

  • Keywords needs-unit-tests added

Replying to dd32:

To trigger this, I'm fairly certain it would need to be meshing with an external object cache, and have the local cache within PHP (The global array) cleared between set/fetch (ie. a different pageload).

Any suggestions on how to do that in the tests? I've just started making tests as of yesterday, so this is all pretty new still. Since my unit test doesn't actually demonstrate the breakage, I'm adding the keyword back.

Cheers,
Derek

#5 @dd32
9 years ago

Any suggestions on how to do that in the tests?

I don't think it's really possible.

One possibility would be to:

  • Require a object-cache.php dropin be loaded and configured (and skip the test if not loaded)
  • Run set(); Flush the PHP cache (by clearing the variable directly); and then running get(); - forcing the data to be retrieved from the external cache

The problem with that, is that the variable that the object cache dropin uses will vary, in the case of Core and Memcache it's $wp_object_cache->cache but others might be different (or have it marked private)

#6 @valendesigns
9 years ago

@dd32 Unfortunately, that sounds fairly complicated and beyond my testing capabilities.

#7 @jamesgol
9 years ago

I've been sick so I haven't had a chance to get back to this but I just looked over the unit tests that @valendesigns wrote and it does look like it covers my test but it is missing the crucial point of modifying the return of a cache_get and comparing the original data with what is in the cache.

For your array-of-objects test you should be able to change it to:

// Verify array of objects set 
wp_cache_set( 'array-of-objects', $expected );
$new = wp_cache_get( 'array-of-objects' );
unset( $new["test"]->test2 );
$this->assertEquals( $expected['test']->test2, 'the' );

I'll attach a simple plugin that demos this with a shortcode.

@jamesgol
9 years ago

Demo of bug using shortcode

@jipmoors
9 years ago

optimization: removing duplicate functionality

#8 follow-up: @jamesgol
9 years ago

@jipmoors the reason that duplicate functionality was in there is because it is more efficient to do it that way.

#9 in reply to: ↑ 8 ; follow-up: @jipmoors
9 years ago

Replying to jamesgol:

@jipmoors the reason that duplicate functionality was in there is because it is more efficient to do it that way.

Could you explain how that would be more efficient? The opcode of the function would be reduced by removing the duplicate functionality.

The suggested benefit would be neglegable and the readability is greatly improved by not repeating the code, in my opinion.

#10 in reply to: ↑ 9 ; follow-up: @jamesgol
9 years ago

Replying to jipmoors:

Could you explain how that would be more efficient? The opcode of the function would be reduced by removing the duplicate functionality.

The suggested benefit would be neglegable and the readability is greatly improved by not repeating the code, in my opinion.

It is more efficient because it reduces the amount of code called. In the best case it is negligible (less than .1% more efficient) but in the more complicated cases when this is actually needed for it is 1-2.9% more efficient (depending on the type of data).

This can potentially be called very frequently, I'm of the opinion that if you are going to slow something down you should minimize that as much as possible. I don't really care what version would be used as long as it goes in, this issue is going to be hit more and more as people are building more complicated apps.

#11 in reply to: ↑ 10 @jipmoors
9 years ago

Replying to jamesgol:

Replying to jipmoors:

Could you explain how that would be more efficient? The opcode of the function would be reduced by removing the duplicate functionality.

The suggested benefit would be neglegable and the readability is greatly improved by not repeating the code, in my opinion.

It is more efficient because it reduces the amount of code called. In the best case it is negligible (less than .1% more efficient) but in the more complicated cases when this is actually needed for it is 1-2.9% more efficient (depending on the type of data).

Adding duplicate functionality only increases the amount of code needing to be parsed to opcode and the only way to know which method is faster is to run some actual tests. But recalling a function -should- be more efficient than repeating the functionality of that function inside itself.

I agree that this is offtopic, but I think it's good to have these kind of discussions to improve the quality of code and coder :)

#12 @desrosj
21 months ago

  • Keywords needs-refresh added
  • Milestone set to Awaiting Review

The patch no longer applies and needs a refresh.

@jamesgol
21 months ago

Updated patch with changes from 2/2020

#13 @jamesgol
21 months ago

Updated patch. I'm not sure if the deep copy function has a better place inside a more utility file/function.

Note: See TracTickets for help on using tickets.