clue/reactphp-redis

Failure to connect is not indicated with rejected promise

JoelFeiner opened this issue · 6 comments

Using essentially this code:

$_factory = new Factory($loop);
$_factory
    ->createClient("redis://$_host:$_port")
    ->then(function(Client $client) use($_host, $_port) {
        $logger->info("Redis connected to $_host:$_port");
    },
    function(\Exception $ex) {
        $logger->error("Failed to connect to Redis:\n$e");
    });

I have Redis turned off, so connections should fail. However, the code is always printing "Redis connected to host:port" instead of "Failed to connect to Redis:...".

The only way I can seem to detect that it failed to connect is to subscribe to the "close" event, which does fire when the connection attempt fails. However, this cannot be so easily included in a promise chain. In fact, since there is no opposite event (we only have close for failure, but not open for success), it's actually impossible to detect if the connection succeeded. The only way I can think of is to attempt a command like PING and chain promises from there. But surely there must be a better way. Am I missing something? Is this something that can be fixed?

clue commented

@JoelFeiner Thanks for reporting. Unfortunately I can not reproduce the problem you're seeing.

This is covered by our test suite and can also be tested by manually launching one of the examples. If it can not connect to your Redis server, this will reject the promise as expected.

When you say you "have Redis turned off", can you verify that you can no longer connect to it? For example, you can also try to create a TCP/IP connection via telnet (and others), e.g. telnet localhost 6379.

@clue This may be a special case. Our Redis instance is behind a loadbalancer (that's another discussion). What I get when I use telnet is this:

$ telnet 127.0.0.1 6379
Trying 127.0.0.1...
Connected to 127.0.0.1.
Escape character is '^]'.
Connection closed by foreign host.

To me, it looks like it makes a connection, which is then immediately terminated. I believe this is thus a problem with the loadbalancer configuration. If I can verify/fix that, I will return and close the issue. If my problem persists, then I will post relevant information.

To clarify, the issue appears to be that the loadbalancer completes the TCP handshake and then immediately closes the connection. This is not detected as a failure in this library. It seems that it only verifies that it can make a connection and then considers that good enough. It doesn't actually verify that it's connected to Redis at all. In fact, I can have it connect to a different service (say, a database) and the library considers this a successful connection to Redis. It looks like this is a weakness in RESP. Thoughts?

clue commented

To clarify, the issue appears to be that the loadbalancer completes the TCP handshake and then immediately closes the connection.

This would indeed be the cause of this issue here. What kind of loadbalancer are you using here? Would using one that follows Redis' semantics be an option in your use case?

It seems that it only verifies that it can make a connection and then considers that good enough.

Correct, Redis protocol (RESP) does not define any inherent semantics for the initial connection phase. What you can do is specify a database number as part of the connection URI string. This will automatically send a SELECT command which would fail in your case because the connection is closed.

While I would more or less consider this a work around, it may still be sufficient to fix this for your use case.

I hope this helps 👍

For posterity (in case anyone stumbles on this issue), I am using Haproxy, which does not have any special Redis protocol handling. I chose to use the option of appending /0 to my URI (e.g. redis://host:port/0). This has solved the issue for me.

Thank you, @clue, for your prompt response and explanations. Much appreciated.

clue commented

@JoelFeiner Thank you for reporting back and happy to see the problem is resolved with the database parameter! Your update is much appreciated, I'm sure this is something more people may run in to.