Relay::close(...) vs Relay::pclose(...) - PhpRedis Compatibility

[Kenneth-Sills]

From the API documentation I see that the Relay class offers these two variant methods for closing a connection. The documentation explicitly says that the close will not close a persistent connection when called, but this seems to break the drop-in compatibility with PhpRedis, who offers a single close(...) that works with both cases and has no pclose(...) equivalent.

Consider the following:

function echoWithActiveFailoverRecovery(): mixed {
    while (true) {
        try {
            return promoteRedisErrors($client->echo('Hi!'));
        } catch (\Exception $exception) {
            if ($exception->getMessage() !== "READONLY You can't write against a read only replica.") {
                throw $exception;
            }

            // The instance we're connected to is now a replicant,
            // close the connection and let the load balancer guide
            // us to the new active instance.
            $client->close();
            usleep(100_000);
        }
    }
}

This kind of functionality is useful when using Redis behind a load balancer like HAProxy, instead of using Sentinel/Clustering/Array management on the client itself.

When using PhpRedis, we will recognize the READONLY failure, kill the connection, and then start a new connection after 100ms - after the load balancer has had time to reroute incoming TCP connections. Failure is graceful.

When using Relay, we will recognize the READONLY failure, then not kill the connection¹, and enter an infinite loop retrying until request timeout.

Footnotes

1. Well, actually, this will have a fatal exception on the retry. A separate issue incoming for that. ↩

[michael-grunder]

Good catch. I don't know why I thought we exposed pclose in PhpRedis. Probably because we do have a similar mechanism in the internals, just not exposed in a method.

Changing Relayto match PhpRedis here would be simple. The only question is whether or not Relay users are now (knowingly or unknowingly) relying on the difference. I suppose either way we technically can change Relay since we're pre 1.0

[tillkruss]

I guess we can act differently depending on the PhpRedis compat mode?

[Kenneth-Sills]

If we could confirm the additional context in #179 (that close(...) will "pseudo-close" a persistent connection), I would imagine it would be safe to have close(...) handle both cases? It seems tough to think of a scenario where someone downstream would all at once be:

1. Explicitly calling close(...) on a persistent connection.
2. Wanting this client to think it was closed (and be unable to recover, given the top-level issue in Relay::close(...) Permanently Closes the Connection (PhpRedis Compatibility) #179).
3. Requiring the socket to silently stay open. Especially since persistent connections are both per-worker and per-client-instance.

[tillkruss]

We'll likely switch to just closing connections in the next release and provide a new method to close and stash a connection if needed.

[tillkruss]

The documentation explicitly says that the close will not close a persistent connection when called, but this seems to break the drop-in compatibility with PhpRedis, who offers a single close(...) that works with both cases and has no pclose(...) equivalent.

So turns out PhpRedis acts just like Relay and vice versa with persistent and non-persistent connections. Except Relay additionally offers pclose() to close persistent connections.

[michael-grunder]

The PhpRedis logic for reference:
https://github.com/phpredis/phpredis/blob/675be9904dee27657ee2f9fc373e0d365ade4aec/library.c#L3483

It's essentially the same. We stash persistent connections unless we are in an unsable state (error conditions, in the middle of a transaction, etc)

[Kenneth-Sills]

Apologies if I'm misunderstanding what you're saying, but in PhpRedis the close(...) method always sets the force parameter flag, so even persistent connections will call php_stream_pclose(...).

Which makes the most sense, as if I explicitly call Redis::close(...), it's because I need the connection dropped and there would be otherwise no way to terminate a connection to (in my use-case) switch from a READONLY replica to the writable master in a cluster behind a reverse proxy without manually recreating the client object (or, effectively the same, manually calling [p]connect(...)).

---

EDIT: I just confirmed this behavior again in a PHP REPL, a Redis monitor, and CLIENT LIST.

$redis = new Redis(['host' => '127.0.0.1', 'persistent' => 'foobar']);
echo $redis->getPersistentId(); // 'foobar'

$redis->echo('BOOP');
// Monitor: [41238] "ECHO" "BOOP"
// CLIENT LIST -> 4747864, port 41238 (age 3, last command 'ECHO')

$redis->close();
// CLIENT LIST ID 4747864 -> No results

EDIT 2: Forgot to use persistence in demo above. 😅 Reran and updated IDs and ports.

[michael-grunder]

Lol I think you're right but I need to verify when I'm back at a computer