php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #59213 Strange behavior in increment on non integer and after
Submitted: 2010-05-13 17:33 UTC Modified: 2011-03-10 20:52 UTC
From: korchasa at gmail dot com Assigned: hradtke (profile)
Status: Closed Package: memcache (PECL)
PHP Version: 5.3.1 OS: Ubuntu 10.4
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: korchasa at gmail dot com
New email:
PHP Version: OS:

 

 [2010-05-13 17:33 UTC] korchasa at gmail dot com
Description:
------------
'../configure'  '--with-fpm' '--with-factcgi' '--with-mcrypt' 
'--with-pear' '--enable-pcntl' '--enable-sysvmsg' '--enable-
sysvsem' '--enable-sysvshm' '--with-mysql=mysqlnd' '--with-
mysqli=mysqlnd' '--with-config-file-path=/etc/php.ini' '--
with-config-file-scan-dir=/etc/php.d' '--with-zlib' '--with-
curl' '--enable-mbstring' '--with-libevent=shared'

Memcached version in my case 1.4.2.

Have the same problem at Gentoo, PHP 5.2.13, pecl-memcache 
3.0.4, memcached 1.3.3-r2.


Reproduce code:
---------------
<?php

$m = new Memcache();
$m->connect('localhost', 11211);

var_dump($m->increment('a', 42));
var_dump($m->get('a'));

$m->set('b', 'bar');
var_dump($m->increment('b', 42));
var_dump($m->get('b'));

$m->set('c', 1);
var_dump($m->increment('c', 42));
var_dump($m->get('c'));

$m->flush();

Expected result:
----------------
42
42
42
42
43
43

Actual result:
--------------
bool(false)
bool(false)
Notice: Memcache::increment(): Server localhost (tcp 11211) 
failed with: CLIENT_ERROR cannot increment or decrement non-
numeric value
 (0) in /home/korchasa/Dropbox/www-
work/localhost/small/memcache.php on line 10
bool(false)
bool(false)
bool(false)
bool(false)


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2010-05-19 16:57 UTC] hradtke@php.net
Confirmed that this happens using your test case.  Will debug and fix.
 [2011-01-24 12:50 UTC] joseph at josephscott dot org
The original report is correct, there is a bug here, but the 
details aren't exactly correct.

After creating a new Memcache object the first block of 
code:

=======
var_dump($m->increment('a', 42));
var_dump($m->get('a'));
=======

The response is:

=======
bool(false)
bool(false)
=======

And that is exactly what you should get, so this works 
correctly.  Because trying to increment a key that hasn't 
been set yet is not valid.  The PHP docs already spell out 
that "Memcache::increment() does not create an item if it 
doesn't already exist."

The second block:

=======
$m->set('b', 'bar');
var_dump($m->increment('b', 42));
var_dump($m->get('b'));
=======

This close, but doesn't exactly work right.  I get:

=======
PHP Notice:  MemcachePool::increment(): Server localhost 
(tcp 11211, udp 0) failed with: CLIENT_ERROR cannot 
increment or decrement non-numeric value
 (0) in /home/akismet/bin/bugs.php on line 12
NULL
bool(false)
=======

The set works.  The increment on a non-numeric value throws 
a notice and returns NULL, which is contrary to the PHP docs 
which state "If item specified by key was not numeric and 
cannot be converted to a number, it will change its value to 
value.".  Instead of NULL for that call I should have got 
back 42 and 'bar' over written with 42 in memcached.

From what I can see NULL should never be returned by 
increment, it either should work and return the new numeric 
value or fail and return FALSE.

Where things get interesting is the third block of code.  By 
itself this block runs fine and does the right thing:

=======
$m->set('c', 1);
var_dump($m->increment('c', 42));
var_dump($m->get('c'));
=======

Gives you :

=======
int(43)
int(43)
=======

However, if you run both the second and third block together 
like this:

=======
$m->set('b', 'bar');
var_dump($m->increment('b', 42));
var_dump($m->get('b'));

$m->set('c', 1);
var_dump($m->increment('c', 42));
var_dump($m->get('c'));
=======

I get:

=======
PHP Notice:  MemcachePool::increment(): Server localhost 
(tcp 11211, udp 0) failed with: CLIENT_ERROR cannot 
increment or decrement non-numeric value
 (0) in /home/akismet/bin/bugs.php on line 15
NULL
bool(false)
bool(false)
bool(false)
=======

We get the NULL and false for working on the 'b' key, but 
then all of the 'c' key operations fail as well.  Inspecting 
the $m object indicates that it still has a connection to 
the memcached server and no connection errors show up.  It 
seems that the resource gets pushed into some unusable state 
where all subsequent interactions fail.

If you look for NULL responses from increment(), then 
disconnect and reconnect then future requests work 
correctly.

Increment is not the only way to push a connection into this 
semi-dead state.  It appears that anything that throws a 
notice about memcached calls will do the trick.  Like 
calling delete($key) instead of delete($key, 0).

Instead of becoming unusable these invalid actions should 
return FALSE and allow future requests to be properly 
handled.
 [2011-01-24 12:53 UTC] joseph at josephscott dot org
Sorry, should have included that this was using pecl-memcache 
3.0.4 as well against memcached 1.4.5
 [2011-01-26 18:15 UTC] vnsavage at gmail dot com
Due to a bug in the older versions of Memcached the value to 
be incremented, was converted to 0. Thus the statement "If 
item specified by key was not numeric and cannot be 
converted to a number, it will change its value to value." 
in the docs  was correct. Now it's not. Memcached will 
return an error on an attempt to increment a value that is 
not numeric.

On the other hand pecl-memcache disconnects from the server 
and tries to make a failover when it sees the error and that 
is why all subsequent requests fail. 

It seems unreasonable to parse the error and call "set" for 
the new value if it was non-numeric, so the docs should be 
changed to reflect that this behavior is no longer valid.

My suggestion is to generate only a notice for the client 
errors and not to do a failover, thus client error 
conditions won't break the memcached connection. I am 
posting a small patch for that:

*================================================*
diff -Naur memcache-3.0.5-orig/memcache_ascii_protocol.c 
memcache-3.0.5/memcache_ascii_protocol.c
--- memcache-3.0.5-orig/memcache_ascii_protocol.c	
2010-10-04 00:41:21.000000000 +0300
+++ memcache-3.0.5/memcache_ascii_protocol.c	2011-01-26 
23:51:39.000000000 +0200
@@ -84,11 +84,13 @@
 	}
 	else if (
 		mmc_str_left(line, "ERROR", line_len, 
sizeof("ERROR")-1) ||
-		mmc_str_left(line, "SERVER_ERROR", line_len, 
sizeof("SERVER_ERROR")-1) ||
-		mmc_str_left(line, "CLIENT_ERROR", line_len, 
sizeof("CLIENT_ERROR")-1)) 
+		mmc_str_left(line, "SERVER_ERROR", line_len, 
sizeof("SERVER_ERROR")-1)) 
 	{
 		response = MMC_RESPONSE_ERROR;
 	}
+	else if (mmc_str_left(line, "CLIENT_ERROR", 
line_len, sizeof("CLIENT_ERROR")-1)) {
+		response = MMC_RESPONSE_CLIENT_ERROR;
+	}
 	else {
 		response = MMC_RESPONSE_UNKNOWN;
 	}
diff -Naur memcache-3.0.5-orig/memcache.c memcache-
3.0.5/memcache.c
--- memcache-3.0.5-orig/memcache.c	2010-10-04 
00:41:21.000000000 +0300
+++ memcache-3.0.5/memcache.c	2011-01-27 
00:29:05.000000000 +0200
@@ -396,7 +396,8 @@
 	}
 
 	/* return FALSE or catch memory errors without 
failover */
-	if (response == MMC_RESPONSE_EXISTS || response == 
MMC_RESPONSE_OUT_OF_MEMORY || response == 
MMC_RESPONSE_TOO_LARGE) {
+	if (response == MMC_RESPONSE_EXISTS || response == 
MMC_RESPONSE_OUT_OF_MEMORY || 
+			response == MMC_RESPONSE_TOO_LARGE 
|| response == MMC_RESPONSE_CLIENT_ERROR) {
 		ZVAL_FALSE(result);
 
 		if (response != MMC_RESPONSE_EXISTS) {
@@ -542,7 +543,7 @@
 		return MMC_REQUEST_DONE;
 	}
 
-	if (response == MMC_RESPONSE_NOT_FOUND) {
+	if (response == MMC_RESPONSE_NOT_FOUND || response 
== MMC_RESPONSE_CLIENT_ERROR) {
 		if (Z_TYPE_P(result) == IS_ARRAY) {
 			add_assoc_bool_ex(result, request-
>key, request->key_len + 1, 0);
 		}
@@ -550,6 +551,11 @@
 			ZVAL_FALSE(result);
 		}
 
+		if (response != MMC_RESPONSE_NOT_FOUND) {
+			php_error_docref(NULL TSRMLS_CC, 
E_NOTICE, "Server %s (tcp %d, udp %d) failed with: %s (%d)",
+				mmc->host, mmc->tcp.port, 
mmc->udp.port, message, response);
+		}
+
 		return MMC_REQUEST_DONE;
 	}
 
@@ -1918,6 +1924,14 @@
 		(*((int *)param))++;
 		return MMC_REQUEST_DONE;
 	}
+	
+	if (response == MMC_RESPONSE_CLIENT_ERROR) {
+		ZVAL_FALSE((zval *)param);
+		php_error_docref(NULL TSRMLS_CC, E_NOTICE, 
"Server %s (tcp %d, udp %d) failed with: %s (%d)",
+				mmc->host, mmc->tcp.port, 
mmc->udp.port, message, response);
+
+		return MMC_REQUEST_DONE;
+	}
 
 	return mmc_request_failure(mmc, request->io, 
message, message_len, 0 TSRMLS_CC);
 }
diff -Naur memcache-3.0.5-orig/memcache_pool.h memcache-
3.0.5/memcache_pool.h
--- memcache-3.0.5-orig/memcache_pool.h	2010-10-04 
00:41:21.000000000 +0300
+++ memcache-3.0.5/memcache_pool.h	2011-01-26 
23:52:22.000000000 +0200
@@ -104,6 +104,7 @@
 #define MMC_RESPONSE_EXISTS			0x02			
/* same as binary protocol */
 #define MMC_RESPONSE_TOO_LARGE		0x03			
/* same as binary protocol */
 #define MMC_RESPONSE_NOT_STORED		0x05			
/* same as binary protocol */
+#define MMC_RESPONSE_CLIENT_ERROR	0x06			
 #define MMC_RESPONSE_UNKNOWN_CMD	0x81			
/* same as binary protocol */
 #define MMC_RESPONSE_OUT_OF_MEMORY	0x82			
/* same as binary protocol */
 
diff -Naur memcache-3.0.5-orig/memcache_session.c memcache-
3.0.5/memcache_session.c
--- memcache-3.0.5-orig/memcache_session.c	2010-10-04 
00:41:21.000000000 +0300
+++ memcache-3.0.5/memcache_session.c	2011-01-27 
00:25:50.000000000 +0200
@@ -426,6 +426,14 @@
 		return MMC_REQUEST_DONE;
 	}
 
+	if (response == MMC_RESPONSE_CLIENT_ERROR) {
+		ZVAL_FALSE((zval *)param);
+		php_error_docref(NULL TSRMLS_CC, E_NOTICE, 
"Server %s (tcp %d, udp %d) failed with: %s (%d)",
+				mmc->host, mmc->tcp.port, 
mmc->udp.port, message, response);
+
+		return MMC_REQUEST_DONE;
+	}
+
 	return mmc_request_failure(mmc, request->io, 
message, message_len, 0 TSRMLS_CC);
 }
 /* }}} */
*================================================*
 [2011-03-10 20:52 UTC] hradtke@php.net
This bug has been fixed in SVN.

In case this was a documentation problem, the fix will show up at the
end of next Sunday (CET) on pecl.php.net.

In case this was a pecl.php.net website problem, the change will show
up on the website in short time.
 
Thank you for the report, and for helping us make PECL better.

Thank you for the patch.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat May 11 21:01:31 2024 UTC