Potential fix for $found issue #40 #43

jenkoian · 2019-12-04T14:30:53Z

There are 2 commits here.

1a5f667
The first one is a stab at fixing #39 - I couldn't get memcache set up on travis though and only managed to do it locally by using a VM. Longer term it probably needs some more thought, maybe docker or something. However, if you have an environment with memcache setup, the tests should run ok. The test added should prove the bug.

5426f2c
This is the suggested fix. It seems very simplistic which makes me a little suspicious, however https://www.php.net/manual/en/memcache.get.php#refsect1-memcache.get-returnvalues confirms that false will be returned if retrieval fails so I think it should be ok. I was worried that we would want to legitimately want to cache false values, but I'm not sure we can?

Assuming you have a local environment (or VM or something) with memcache all set up, here's how I suggest reviewing this PR (note I haven't tested all of these commands):

Check out the PR: git fetch origin +refs/pull/43/merge && git checkout FETCH_HEAD
Run composer (or use global phpunit if you'd prefer): composer install
Revert the second commit: git revert 5426f2c
Run tests: vendor/bin/phpunit (or just phpunit if you have it globally)
Notice failure verifying the bug
Reset the branch: git reset
Run tests again: : vendor/bin/phpunit (or just phpunit if you have it globally)
Notice tests now pass

nickdaugherty · 2019-12-05T00:04:41Z

Thanks for adding all the tests @jenkoian, that's great!

For the fix - I believe it's not actually fixing the underlying problem, b/c it's returning false for $found when the value was false - the $found param exists to distinguish the two, but currently the code ties them back together without actually tracking if it was found in memcached or not.

I think what we need to do is rework the internal cache mechanism, to store items as an associative array along with their $found status. Currently it's one dimensional so we can only store the value for each key, not "extras" like $found.

jenkoian · 2019-12-05T13:46:41Z

Thanks for the feedback @nickdaugherty I've added a new commit which I think is the kind of thing you mean?? I need to add a bunch more tests to verify this first, but wanted to get your initial feedback on direction. Thanks!

nickdaugherty · 2019-12-09T22:53:24Z

Yep you got it @jenkoian - that's exactly what I was meaning. Since it's a pretty large change to the internals, we'll want to be sure the tests cover as many cases as possible.

jenkoian · 2019-12-10T17:04:05Z

Cool, so I've started adding tests. I've added tests for the add method so far. I'll continue to add tests as and when I can. There's a few things which are going to be tricky to test without some bigger refactorings, I'll leave them for now and maybe we can reflect on them at the end.

Things not tests on add()

The unsetting of local cache on memcache failure (I think this would involve refactoring to inject memcache so we can inject a mock for testing)

jenkoian · 2019-12-19T15:10:00Z

object-cache.php

-		$this->cache[ $key ] = $data;
+		$this->cache[ $key ] = [
+			'value' => $data,
+			'found' => false, // Set to false as not technically found in memcache at this point.


@nickdaugherty are you able to confirm if this should be true or false at this point?

Thinking about this, I'm wondering if we set to false here (in case someone is using no_mc_groups but then if the set is successful further down, we update the found in the local cache to be true? WDYT?

but then if the set is successful further down, we update the found in the local cache to be true

This sounds like the way to go to me.

Done in latest commit.

jenkoian · 2019-12-19T15:11:37Z

Hi @nickdaugherty,

Been working on the tests any chance I've had and now think the tests should be pretty comprehensive. There are cases where I don't think a test could be written without larger scale refactoring, so I've marked those tests as skipped.

Are you able to have a look, maybe run the tests locally and let me know if you think this is good to go?

Things still left to do:

Squash commits
Get travis set up

jenkoian · 2019-12-20T15:29:02Z

Update: Have added a travis config file (you can see a passing build here jenkoian#1 don't have the permissions to set it up for this repo) and squashed the commits.

Only thing outstanding I think is a response on this: #43 (comment)

Thanks!

nickdaugherty · 2020-01-31T17:37:08Z

Hey @jenkoian - sorry for the delay - just wanted to let you know this is actively being worked on.

nickdaugherty · 2020-01-31T18:57:28Z

@jenkoian this is looking really great, thank you for all your hard work on it. The test coverage in particular is 💯

I'm having trouble getting the tests running locally - to get Travis going on the repo, can you open a new PR that includes just the test scaffolding + .travis.yml file? That way we can have Travis pre-configured and then can let it run the tests for this PR (rather than waiting until merge to have Travis give a 👍 / 👎 ).

jenkoian · 2020-01-31T20:34:04Z

Yep! Will do that now

jenkoian · 2020-01-31T20:45:49Z

Here you go: #44 once merged I can rebase this if you need 👍

This will allow us to update the $found parameter accurately when used. Also added a bunch of tests to make sure this doesn't cause regression and added a travis config file so tests can be run on travis.

jenkoian · 2020-01-31T22:11:07Z

@nickdaugherty ok, rebased this against master and sorted a few rough edges. Let me know if you want me to squash commits or anything. Thanks!

nickdaugherty

Looks great, thanks so much for working on this!

jenkoian mentioned this pull request Dec 4, 2019

$found parameter to wp_cache_get() is incorrect when object is in local cache #40

Closed

jenkoian commented Dec 19, 2019

View reviewed changes

jenkoian force-pushed the found-fix branch from f196049 to 6ff2698 Compare December 20, 2019 15:27

jenkoian mentioned this pull request Dec 20, 2019

Update wp-memcached submodule. Automattic/vip-go-mu-plugins#1350

Closed

4 tasks

jenkoian mentioned this pull request Jan 31, 2020

Adding tests. #44

Merged

Ian Jenkins added 3 commits January 31, 2020 22:07

Add unit test for bug where found is giving incorrect results.

bf25bbe

Store the found value separately to the value.

276c81b

This will allow us to update the $found parameter accurately when used. Also added a bunch of tests to make sure this doesn't cause regression and added a travis config file so tests can be run on travis.

Update found value to true after set() in memcache.

4fe1eb8

jenkoian force-pushed the found-fix branch from 6cd5d95 to 4fe1eb8 Compare January 31, 2020 22:10

Ian Jenkins added 2 commits January 31, 2020 22:12

Remove incorrect script in favour of bash script

37562cf

Correct some indents after conflict resolution

9a6509f

jenkoian force-pushed the found-fix branch from 04073c3 to 9a6509f Compare January 31, 2020 22:17

nickdaugherty approved these changes Jan 31, 2020

View reviewed changes

nickdaugherty merged commit 7011c7a into Automattic:master Jan 31, 2020

jenkoian deleted the found-fix branch February 4, 2020 15:30

dd32 mentioned this pull request Jun 18, 2020

$found parameter to wp_cache_get() is incorrect when object is in local cache in a non-persistent group #61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential fix for $found issue #40 #43

Potential fix for $found issue #40 #43

jenkoian commented Dec 4, 2019 •

edited

Loading

nickdaugherty commented Dec 5, 2019 •

edited

Loading

jenkoian commented Dec 5, 2019

nickdaugherty commented Dec 9, 2019

jenkoian commented Dec 10, 2019

jenkoian Dec 19, 2019

jenkoian Dec 20, 2019

nickdaugherty Dec 20, 2019

jenkoian Dec 21, 2019

jenkoian commented Dec 19, 2019 •

edited

Loading

jenkoian commented Dec 20, 2019

nickdaugherty commented Jan 31, 2020

nickdaugherty commented Jan 31, 2020 •

edited

Loading

jenkoian commented Jan 31, 2020

jenkoian commented Jan 31, 2020

jenkoian commented Jan 31, 2020 •

edited

Loading

nickdaugherty left a comment

Potential fix for $found issue #40 #43

Potential fix for $found issue #40 #43

Conversation

jenkoian commented Dec 4, 2019 • edited Loading

nickdaugherty commented Dec 5, 2019 • edited Loading

jenkoian commented Dec 5, 2019

nickdaugherty commented Dec 9, 2019

jenkoian commented Dec 10, 2019

jenkoian Dec 19, 2019

Choose a reason for hiding this comment

jenkoian Dec 20, 2019

Choose a reason for hiding this comment

nickdaugherty Dec 20, 2019

Choose a reason for hiding this comment

jenkoian Dec 21, 2019

Choose a reason for hiding this comment

jenkoian commented Dec 19, 2019 • edited Loading

jenkoian commented Dec 20, 2019

nickdaugherty commented Jan 31, 2020

nickdaugherty commented Jan 31, 2020 • edited Loading

jenkoian commented Jan 31, 2020

jenkoian commented Jan 31, 2020

jenkoian commented Jan 31, 2020 • edited Loading

nickdaugherty left a comment

Choose a reason for hiding this comment

jenkoian commented Dec 4, 2019 •

edited

Loading

nickdaugherty commented Dec 5, 2019 •

edited

Loading

jenkoian commented Dec 19, 2019 •

edited

Loading

nickdaugherty commented Jan 31, 2020 •

edited

Loading

jenkoian commented Jan 31, 2020 •

edited

Loading