increment function working incorrectly (with binary protocol)

[parf]

5.4.3 64 bit

increment / decrement operations failing with binary protocol in the following
scenario.

add operation returns incorrect result (true instead of false)

there is no bug without binary protocol

test script (you can run it as php script also)

!/bin/env spartan-test

// spartan-test unit test: https://github.com/parf/spartan-test
// PHP memcached extension BUGS

// you can run this test
// it shows just found bug in memcached php extension for php

$mc = new Memcached();

={"class":"Memcached","0":{}}

; $mc->addServer("localhost", 11211);

// this IS a bug option - binary protocol messes things up
; $mc->setOption(Memcached::OPT_BINARY_PROTOCOL, true);

$mc->getVersion();

={"localhost:11211":"1.4.13"}

// #={"p:11211":"1.4.10"} << test on another server also have same issue

phpversion();

="5.4.1"

// rpm -qa | grep php-pecl-memcached : php-pecl-memcached-2.0.1-6.el6.remi.x86_64

; $key="key-for-testing";

// OK CASE
$mc->set($key, 0);

=true

$mc->increment($key);

=1

$mc->increment($key);

=2

$mc->increment($key);

=3

// BUG CASE
// extra get after set messes subsequent increments
$mc->set($key, 0);

=true

$mc->get($key);

=0

// OMG - BUG !!!
$mc->increment($key);

=-1

$mc->increment($key);

=1

$mc->increment($key);

=2

// ANOTHER BUG
; $mc->delete($key);

$mc->add($key, "abc");

=true

$mc->get($key);

="abc"

// BUG - should return false !!!
$mc->add($key, "def");

=true

$mc->get($key);

="abc"

[bvis]

With decrement it's the same:

addServer('localhost', '11211'); $m->setOption(Memcached::OPT_BINARY_PROTOCOL, true); $key = uniqid('testIncrement'); assert($m->add($key, 7, 2) === true); // Adding value 7 assert($m->get($key) === 7); // Check right value was saved assert($m->decrement($key, 2) === 5); // Decrement to 5 Shows a warning in the last assert.

[kixorz]

We're experiencing the same issue here.

[doapp-ryanp]

This is blocker for me - anyone have a workaround for this? If you do any operation before the increment/decrement it will always return -1:

php-memcached v2.1.0, libmemcached 1.0.15, ubuntu 12.04 64bit.

ex:

<?php
$m = new Memcached();
$newServers = array(
                  array('localhost', 11211, 100),
              );            
$m->setOption(Memcached::OPT_BINARY_PROTOCOL, true);

$m->addServers($newServers);

$d = $m->get('foo');

$m->set('counter', 5);
$n = $m->decrement('counter');  //should return 4, returns -1!?!?
var_dump($n);

$n = $m->decrement('counter', 10);
var_dump($n);

var_dump($m->get('counter'));

$m->set('counter', 'abc');
$n = $m->increment('counter');
var_dump($n);

Output is:

int(-1) int(4) int(0) int(-1)

[mhagstrand]

I have a workaround. Using getMulti() instead of get() stops this issue from occurring. The code path for getMulti() and get() are very different. From bvis's example we can see the bug occur.

<?php

$m = new Memcached();
$m->addServer('localhost', '11211');
$m->setOption(Memcached::OPT_BINARY_PROTOCOL, true);

$key = uniqid('testIncrement');
assert($m->add($key, 7, 2) === true); // Adding value 7
assert($m->get($key) === 7); // Check right value was saved
assert($m->decrement($key, 2) === 5); // Decrement to 5

However this code works and does not have the bug:

<?php

$m = new Memcached();
$m->addServer('localhost', '11211');
$m->setOption(Memcached::OPT_BINARY_PROTOCOL, true);

$key = uniqid('testIncrement');
assert($m->add($key, 7, 2) === true); // Adding value 7
$retVal = $m->getMulti(array($key)); // Check right value was saved
assert($retVal[$key] === 7); // Check right value was saved
assert($m->decrement($key, 2) === 5); // Decrement to 5

The function php_memc_get_impl() in php_memcached.c uses the libmemcached function memcached_fetch(). The function memcached_fetch() is deprecated:

http://docs.libmemcached.org/libmemcached/memcached_fetch.html

I believe memcached_fetch() is causing this problem but I will have to do more research to confirm.

[doapp-ryanp]

Thanks for the update. I have modified my app code to do what you mention and it seems to be working.

[trvrnrth]

I haven't run the tests here explicitly but I believe pull request #75 should fix this.