libmemcached 1.0.16 is totally incompatible with session.lazy_write

[aeijdenberg]

If session.lazy-write is enabled (which it is by default in PHP 7) calls to session-write-close() will check if the session is modified, and if not will call PS_UPDATE_TIMESTAMP_FUNC in php-memcached which in turn calls memcached_touch().

In libmemcache 1.0.15, this was broken because when reading the response, it ignored the 4 bytes returned in the body by the server, which caused the next request to fail. See bug 1104642.

This was attempted to be fixed with the following in 1.0.16. This is also broken, because it ignores the bodylen and instead tries to read 32 bytes (bodylen is normally 4 in this context):

-        WATCHPOINT_ASSERT(bodylen == 0);
+        if (bodylen != 0)
+        {
+          char touch_buffer[32]; // @todo document this number
+          rc= memcached_safe_read(instance, buffer, sizeof(touch_buffer));
+        }
         return MEMCACHED_SUCCESS;

What this means is that in 1.0.16, calls to TOUCH succeed, but if successful, they always take 5 seconds, while it times out waiting for the remaining 28 bytes of data. It also swallows the error, so that the caller thinks it succeed, just very slowly.

To trigger this, use PHP 7 and libmemcached 1.0.16:

<?php

ini_set('session.save_handler', 'memcached');
ini_set('session.save_path', "memcache:11211");
ini_set('memcached.sess_prefix', "foo");
ini_set('memcached.sess_locking', '1');

session_id("foo");

$start1 = microtime(true);
session_start();
$_SESSION["a"] = "b";
session_write_close();
$end1 = microtime(true);

$start2 = microtime(true);
session_start();
session_write_close();
$end2 = microtime(true);

echo "Time to open / close session:".round($end1 - $start1, 3)."\n";
echo "Time to open / close session:".round($end2 - $start2, 3)."\n";

The second time will always be just over 5 seconds - this bug only fires if the key already exists.

Looking further, it seems like 1.0.17 attempts to fix the breakage caused in 1.0.16:

+        rc= MEMCACHED_SUCCESS;
+        if (bodylen == 4) // The four byte read is a bug?
+        {
+          char touch_buffer[4]; // @todo document this number
+          rc= memcached_safe_read(instance, touch_buffer, sizeof(touch_buffer));
+        }
+        return memcached_set_error(*instance, rc, MEMCACHED_AT);

While this does seem to work, note that if the server ever does decide to return a number of bytes not equal to 4 or 0, then this will regress back to the same error condition as 1.0.15, where success is returned, and the bytes will be incorrectly read as part of the next response.

Looks like this bug reared it's head here too a while back, the result being that 1.0.14 was dropped.

I mention all this as I believe 1.0.16 is the default on CentOS7 and I note the README in this repo says that 1.0.16 or higher is recommended.

Not sure what best course of action is... but I feel better for reporting it. Maybe just an update to the README and a regression test based on the above? Maybe fail fast if lazy sessions are detected to be enabled with an incompatible version of libmemcached?

Note I think the bug can be avoided by using ASCII mode for TOUCH.

[sodabrew]

Thank you for the detailed bug report!!

I could simply adjust the README to state that libmemcached 1.0.18 is always recommended, and that's been the last published version for a few years now.

[carlwgeorge]

@sodabrew Do you feel like this is a severe enough problem to set a hard minimum requirement of libmemcached 1.0.18?

[sodabrew]

Setting a minimum libmemcached 1.0.18 would write off a number of distribution LTS releases that are currently active. Not everyone uses sessions, so that would be a widespread inconvenience for a very limited benefit.

The release notes do state that 1.0.18 is strongly recommended, but README still says 1.0.16, I just fixed it.

[carlwgeorge]

That is exactly the context I'm dealing with. I'm a maintainer of the IUS project. We currently ship php56u-pecl-memcached (built against PHP 5.6). It uses stock libmemcached 1.0.16 on EL7, but we also maintain a libmemcached10 compatibility package (also version 1.0.16) for EL6 due to the much older stock library. Now that memcached has PHP7-compatible releases (thanks a bunch for that by the way), I'm working on new php70u-pecl-memcached and php71u-pecl-memcached packages. I'm trying to decide if I should continue using the same build requirements (libmemcached10 1.0.16 on EL6 and libmemcached 1.0.16 on EL7), or if I should update our libmemcached10 package to 1.0.18 and use it everywhere. If 1.0.16 is good enough, I'm inclined to use the stock libmemcached on EL7.

Sorry to hijack your bug report @aeijdenberg.

[aeijdenberg]

You're most welcome to hijack in whatever manner works best.

An options that avoid this bug surfacing, is to have the application which is calling us set session.lazy_write to false.That should stop this code path from being called.

Not sure if it's possible to detect this condition (lazy_write enabled, libmemcached < 1.017) inside of code and print a warning to php error logs?

[sodabrew]

Yes, we can definitely detect and warn in that condition!

[sodabrew]

Does the warning in #322 match the error combination to warn about?

[aeijdenberg]

Yes, that looks right to me. Thanks for adding!

[viable-hartman]

I built libmemcached on CentOS 7 from the 1.0.18 source, and I still get the "touch" warning. I've also included the libmemcached-devel RPM I built from source thinking this fix read the version from the header files, but it seems to make no difference. I still get the warning in my logs.

Any help as to why?

[viable-hartman]

ldd on php-memcached lib shows it is using my 1.0.18 library, and "strings | grep -A 1 -B 1 " does show version 1.0.18, so its definitely the proper version.

This fix doesn't seem to realize that, however as it still puts the, "using touch command with binary protocol is not recommended with libmemcached versions below 1.0.18, please use ascii protocol or upgrade libmemcached," in my log file over and over.

So can anyone tell me exactly where LIBMEMCACHED_VERSION_HEX is being set, as it doesn't seem to be getting the right value on my servers somehow?