Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confirm memcached is giving the correct response #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

WPprodigy
Copy link

@WPprodigy WPprodigy commented Feb 8, 2021

If you send an array, the memcache client will respond with an associative array: https://www.php.net/manual/en/memcache.get.php. With this, we can ensure that the memcached server is giving us the correct response.

This is just a proof of concept for now. Some further ideas include:

  • Checking if it returns an array but with the wrong key, and log an error and retry once if so (helping keep tabs on the scale of the problem we're dealing with).
  • Doing this in the memcache php client extension instead.

As a side note, we should really be making use of this functionality for the multi_get() logic that WP now uses - as it can greatly limit the # of roundtrips to the memcached server. But can come back to that another day :).

Copy link
Contributor

@rinatkhaziev rinatkhaziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isset is not reliable in this context because it'll be false if the value is NULL:

$keys = [
  'test' => null,
  'test2' => 'null',
  'test3' => 1,
];

foreach( $keys as $key => $val ) {
  var_dump( isset( $keys[ $key ] ) );
}
echo "\n";
foreach( $keys as $key => $val ) {
  var_dump(array_key_exists( $key , $keys ) );
}

=>

bool(false)
bool(true)
bool(true)

bool(true)
bool(true)
bool(true)

array_key_exists would be safer.

@WPprodigy
Copy link
Author

WPprodigy commented Feb 10, 2021

Documenting some of the edge-case behaviors:

## Non-existing keys

$flags = false;
$value = $memcache->get( KEY_THAT_DOES_NOT_EXIST, $flags );
var_dump( $flags, $value );
> bool(false) bool(false)

$flags = false;
$value = $memcache->get( [ KEY_THAT_DOES_NOT_EXIST ], $flags );
var_dump( $flags, $value );
> bool(false) array(0) { }
## Key with empty string value

$flags = false;
$value = $memcache->get( EMPTY_KEY, $flags );
var_dump( $flags, $value );
> int(0) string(0) ""

$flags = false;
$value = $memcache->get( [ EMPTY_KEY ], $flags );
var_dump( $flags, $value );
> array(1) { ["emptykey"]=> int(0) } array(1) { ["emptykey"]=> string(0) "" }
## Key with empty array value

$flags = false;
$value = $memcache->get( EMPTY_ARRAY_KEY, $flags );
var_dump( $flags, $value );
> int(1) array(0) { }

$flags = false;
$value = $memcache->get( [ EMPTY_ARRAY_KEY ], $flags );
var_dump( $flags, $value );
> array(1) { ["emptyarraykey"]=> int(1) } array(1) { ["emptyarraykey"]=> array(0) { } }
## Key with null value

$flags = false;
$value = $memcache->get( NULL_KEY, $flags );
var_dump( $flags, $value );
> int(1) bool(false)

$flags = false;
$value = $memcache->get( [ NULL_KEY ], $flags );
var_dump( $flags, $value );
> array(1) { ["nullkey"]=> int(1) } array(1) { ["nullkey"]=> NULL }
## Key with false value (same behavior with `true`, just flipped of course)

$flags = false;
$value = $memcache->get( FALSE_KEY, $flags );
var_dump( $flags, $value );
> int(256) bool(false)

$flags = false;
$value = $memcache->get( [ FALSE_KEY ], $flags );
var_dump( $flags, $value );
> array(1) { ["falsekey"]=> int(256) } array(1) { ["falsekey"]=> bool(false) }
## Key with normal string value

$flags = false;
$value = $memcache->get( CANARY_KEY, $flags );
var_dump( $flags, $value );
> int(0) string(6) "canary"

$flags = false;
$value = $memcache->get( [ CANARY_KEY ], $flags );
var_dump( $flags, $value );
> array(1) { ["canary"]=> int(0) } array(1) { ["canary"]=> string(6) "canary" }
  • ✅ All that matters for $flags is that it returns false for "Non-existing keys" and for that only. So all good there .
  • 🟥 NULL_KEY is a problem, as previously it would have just been seen as false but now it's NULL. Needs adjustment.
  • ✅ The "Non-existing keys" differences are okay, as $found will cover that difference and set value to false anyway.
  • ✅ Everything else ports over well.

@WPprodigy
Copy link
Author

WPprodigy commented Feb 11, 2021

  • Added logging so we can better track how often inconsistencies occurs.
  • Handled the NULL edge case for backwards-compatibility.
  • get_multi() didn't have a very accurate implementation for $found. Does now :)

The PR is a bit more complex now though, but some good news is that we can eventually deploy to object-cache-next.php first and conditionally test before a full rollout.

Side note: Held off on using PHP's new array destructuring & array_key_first(), as I'm not sure what minimum php version is being targeted here.

@WPprodigy WPprodigy changed the title Proof of concept: Confirm memcached is giving the correct response Confirm memcached is giving the correct response Feb 11, 2021
Edge cases OP.

”i before e” they say…
Slight performance boost under optimal conditions.
@sonarcloud
Copy link

sonarcloud bot commented Jun 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants