Support the stream path in the connection params and URI (for persistent connections with database initCmd)

[dominics]

Here's a quick example test for the interaction between the database connection parameter, and persistent connections:

<?php
$cache_client = new Predis\Client('tcp://127.0.0.1:6379?persistent=1&database=0', []);
$durable_client = new Predis\Client('tcp://127.0.0.1:6379?persistent=1&database=1', []);

$cache_client->ping();    # Connection 1, "SELECT" "0", db0 "PING"
$durable_client->ping();  # Connection 1, "SELECT" "1", db1 "PING"
$cache_client->ping();    # Connection 1, db1 "PING" <<< This should have been on db0
$durable_client->ping();  # Connection 1, db1 "PING"

As you can see, initCmds are only run once, upon a connection being established. Then, because the two clients are connecting to the same IP address and port combination, with persistent connections turned on, they share a single persistent connection. The result is that the third ping() is executed in the wrong database (although this is particularly bad when the command is flushdb instead of ping :-)

The PHP manual user notes (sigh) for stream_socket_client mention that it actually supports specifying a path, and that this will make it open separate persistent connections:

The remote_socket argument, in its end (well... after the port), can also contain a "/" followed by a unique identifier. This is especially useful if you want to create multiple persistent connections to the same transport://host:port combo.

This seems to work. With this PR applied, and specifying different paths in the above example (say, tcp://127.0.0.1:6379/first?persistent=1&database=0 and tcp://127.0.0.1:6379/second?persistent=1&database=1), separate persistent connections are opened, and we can happily have one for each database. Then the SELECT in the initCmds will happily stick around, and won't be overwritten.

[dominics]

Might it be appropriate to build the path out of the alias if one isn't supplied? I don't know, but it seemed like a bigger change to me (might break intentional sharing of the underlying connection via two different aliases), whereas this is a backward compatible feature addition.

[nrk]

I wouldn't use alias for the same reasons you suggested, but at the same time using the path part of the URI is counterintuitive:

• the path part influencing persistent connections is just a (weird) implementation detail of PHP for stream resources (furthermore, not all of the connection backends rely on streams).
• when using a named array for parameters instead of an URI string you'd need to add the path field which doesn't ring any bell in that context.

I think a different parameter should be used in this case, but I can't think of anything decent right now.

[dominics]

Some thoughts:

• The path parameter is already used for 'unix' scheme stream connections. If you want to use the 'unix' scheme with a named parameter array, you'll need to add the 'path' field already. And that's even the case for non-stream connection backends that support unix sockets (like PhpiredisConnection).
  • I do understand that path has a much clearer meaning for unix sockets than for TCP sockets, but I don't think that's a good reason to strip the path for TCP sockets. I certainly think it's more surprising that the path gets stripped than if it weren't.
• It might not be necessary to add it as having a default value in ConnectionParameters (because it seems to work without the default for the existing unix-based streams), in which case the change is only to the TCP stream initializer. Making a change to the specific stream initializer that uses stream_socket_client to support a quirk of stream_socket_client makes sense to me.
• Sure, the path param has quite a different meaning for unix sockets as opposed to the proposed change for TCP. And for non-persistent TCP connections, path has very little meaning. But the "implementation detail" of the path affecting persistent connections needn't be something you add to the public contract of 'path' in Predis; it can stay as something that just "happens" to work.

[nrk]

After thinking about it I think I'll accept your request as is, but using a slightly different approach.

I prefer not to add a default value for path in the connection parameters class, but we can augment $uri in StreamConnection::tcpStreamInitializer() when we find that $parameters->persistent is TRUE:

$uri = "tcp://{$parameters->host}:{$parameters->port}";

if (isset($parameters->persistent) && $parameters->persistent) {
    $flags |= STREAM_CLIENT_PERSISTENT;
    $uri .= strpos($path = $parameters->path, '/') === 0 ? $path : "/$path";
}

A couple of notes:

• When the user provides an incorrect path without a leading slash (could happen with named arrays), we add it silently to avoid generating a broken URI (e.g. $params = ['persistent' => true, 'path' => 'first'] would produce tcp://127.0.0.1/6379first with your proposed patch).
• We don't care about path when not using persistent connections so we can just skip it (even the slash) when $parameters->persistent is not set or FALSE.

Would it be OK?
Aside from the master branch, I can merge this change into the stable branch to get this feature included in v0.8.5.

[nrk]

Change is available on both master and v0.8, will make it into v0.8.5 when will be released in just a few days.

Thanks @dominics.

[dominics]

Fantastic, thanks!