Memcache fails to connect when using Unix sockets and PECL Memcached [#1690130]

I'm not sure if this is a feature request or a bug, so feel free to reassign it if necessary.

When the PECL Memcached extension is used with Unix sockets, Drupal fails to connect to the Memcached daemon. This is due to the inconsistency between how the sockets should be specified in $memcache->addSever(). PECL Memcache expects: unix:///path/to/socket while PECL Memcached expects /path/to/socket.

The only point of reference I could find to this was in the PECL Memcached source on GitHub. Support for sockets was added to PECL Memcached in version 2.0.0b1 which was released on 2011-03-13. The current stable version is 2.0.1 released on 2012-03-03.

The attached patch attempts to solve this problem, though I am really open for suggestions as I think it is currently a bit messy. It seems to be working fine for me on a test setup, the Memcache Admin module lists hits when set to display on each page, and the Memcached daemon statistics concur. I have not tested locking apart from enabling it and it seems to be running without errors.

This does leave the Memcache Admin overview page mostly non-functional, and a related issue for the admin module when using sockets, which would also need a new way of dealing with PECL Memcached socket paths, is #1547390: Unix sockets confuse the stats page

And thanks for the module, it helps substantially on a VPS when disk i/o is stressed by other VM's. This is despite a large innodb buffer pool and innodb_flush_log_at_trx_commit = 2.

Comment	File	Size	Author
#12	memcache-allow-memcached-sockets-1690130-12.patch	4.45 KB	Phizes
#7	memcache-unix-sockets-1690130-7.patch	3.76 KB	markpavlitski
#3	patch_fix_stat_page_when_using_PECL_Memcached.patch	87 bytes	cn_gd
	allow-pecl-memcached-sockets.patch	1.92 KB	Phizes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Phizes CreditAttribution: Phizes commented 18 July 2012 at 15:06

Status:

Active

» Needs review

Correcting status

Comment #2

cn_gd CreditAttribution: cn_gd commented 8 August 2012 at 05:07

I'm using PECL Memcached 2.1.0,apply the patch but still fail to connect.

Comment #3

cn_gd CreditAttribution: cn_gd commented 8 August 2012 at 07:15

File	Size
patch_fix_stat_page_when_using_PECL_Memcached.patch	87 bytes

fixed!

when you haved apply the patch above,you should also apply
The patch(http://drupal.org/files/patch-memcache-memcache_admin-socket-support_0.p...) in http://drupal.org/node/1547390

but you shoud change the patch this way:

+ if (strncmp($server, 'unix://', 7) == 0) {
+ $server .= ':0';
+ }

should be

+ if (strncmp($server, 'unix://', 7) == 0) {
+ $server = substr($server,7).':0';
+ }

that works for me.

the patch file:

Comment #4

Phizes CreditAttribution: Phizes commented 9 August 2012 at 19:40

This would not work for both PECL Memcache and PECL Memcached, the patch in comment 3 above will break the memcache_admin module for PECL Memcache users.

In the original issue above:

PECL Memcache expects: unix:///path/to/socket while PECL Memcached expects /path/to/socket.

The patch in the issue summary is only meant to address the memcache module and not memcache_admin.

Comment #5

cn_gd CreditAttribution: cn_gd commented 28 August 2012 at 07:45

but it works for me.

Comment #6

Phizes CreditAttribution: Phizes commented 29 August 2012 at 10:01

The PECL Memcache extension would not work, it works for you because you are using the PECL Memcached extension. They are 2 separate projects and they each expect a different string to define the socket connection.

Despite this, this issue has nothing to do with the memcache_admin module, I have not tried to fix it there yet, as I am waiting on feedback for this issue. This is just for the main memcache module.

Comment #7

markpavlitski CreditAttribution: markpavlitski commented 17 April 2013 at 10:47

File	Size
memcache-unix-sockets-1690130-7.patch	3.76 KB

The attached patch adds unix socket support for both PECL extensions.

It also allows the port number to be ignored, defaulting to 0 for sockets and 11211 for port connections.

For compatibility with existing installations and with both extensions, any of the following formats may be used for the host:


$conf['memcache_servers'] => array(
  '/path/to/memcache.socket' => 'cluster',
  '/path/to/memcache.socket:0' => 'cluster',
  'unix:///path/to/memcache.socket' => 'cluster',
  'unix:///path/to/memcache.socket:0' => 'cluster'
);

Comment #8

Phizes CreditAttribution: Phizes commented 17 April 2013 at 11:31

With regards to #7, the patch in the original issue description already fixed it for both extensions as it wasn't broken for PECL Memcache, it just added support for PECL Memcached.

I'm not sure that adding all of that configuration complexity to account for a bug in the past is really a good way to go about this? It just seems strange to me to add code to a project to support a workaround that may be in use for a bug. I guess this is only really something the module maintainer can answer though.

Comment #9

markpavlitski CreditAttribution: markpavlitski commented 17 April 2013 at 12:40

@Phizes The example is showing 4 alternatives, rather than 4 required configurations, and perhaps is better shown as:

$conf['memcache_servers'] => array('/path/to/memcache.socket' => 'default');
$conf['memcache_servers'] => array('/path/to/memcache.socket:0' => 'default');
$conf['memcache_servers'] => array('unix:///path/to/memcache.socket' => 'default');
$conf['memcache_servers'] => array('unix:///path/to/memcache.socket:0' => 'default');

I don't think that it adds any configuration complexity and will work with the configuration in your original post.

Both PECL extensions support different mechanisms for socket connections, and the patch supports both mechanisms to reduce possible confusion when using the Memcache extension and documentation.

Your original patch only supports the PECL Memcache method of specifying a socket, i.e. unix:///path/to/socket. I would suggest that either the PECL Memcached method should also be supported, i.e. /path/to/socket, or that README.txt should be updated to specify that only the one method can be used.

The documentation currently also specifies that a host and port are both required, however the connection fails when specifying a port using your patch (for both extensions), i.e. 'unix:///path/to/socket:0'.

Comment #10

Phizes CreditAttribution: Phizes commented 17 April 2013 at 13:31

My point about the complexity was the code overhead required for backwards compatibility with a bug workaround.

My original patch specifically deals with Memcached, the issue title indicates this, and if you reread my code you will see I account for both the different cases and, if you reread the issue summary, I also explain the differences between what the libraries expect. Memcache was always supported, so why would I write a patch to add Memcache support?

Taken from the module itself:
// Support unix sockets in the format 'unix:///path/to/socket'.
I just stuck to this convention, and as you can see above it does not include a port, which is completely pointless if it is just going to be set to 0 anyway. So naturally it would break when configured incorrectly. The documentation may state a port is required, but seeing as Unix sockets do not deal with ports, this does not make sense apart from manually providing 0 as a place holder which is already handled automatically by the module.

My whole point is that the argument for supporting the different configuration methods is for backwards compatibility with a bug workaround, this does not seem sensible to me.

Edit: Why should we account for different configurations which the underlying libraries support? Generally it seems normal to abstract away this sort of thing, I mean there are modules dedicated to providing the exact same API for different libraries as is.

Comment #11

markpavlitski CreditAttribution: markpavlitski commented 17 April 2013 at 15:20

Excluding comments and documentation, both patches are very similar in length, so I'm not sure what you are referring to in terms of the additional code overhead.

The patch in #7 is not implementing backwards compatibility foran old bug, it is providing support for the two available specifications for describing a socket.

The PECL Memcached documentation states that for socket support you should use /path/to/socket, however this will not work with your patch, which is confusing for users (who shouldn't be expected to dig through the code comments to get the module to work).

The fix for this issue should either work in the expected manner (as per the existing PECL documentation) or should provide additional documentation if this is not the case.

Comment #12

Phizes CreditAttribution: Phizes commented 17 April 2013 at 17:17

File	Size
memcache-allow-memcached-sockets-1690130-12.patch	4.45 KB

Taking line deletions into account (also discounting comments and documentation), my patch adds 5 lines of code whereas as yours adds 18 lines, I don't see that as roughly similar.

It seems I misunderstood you when you said "For compatibility with existing installations ...". Again, I do not see why the underlying library requirements should affect what the Drupal module expects at all. Additionally at the time I went looking for what exactly Memcached expected, I found it in the source on GitHub, and it's not commented there.

If you have a link for documentation which explains Memcached usage in greater detail, could you please share it as I am not sure what some of the constants do.

Attached is an updated patch of the one in the issue summary which documents how to specify Unix sockets.