mariano / disque-php

PHP library for Disque, an in-memory, distributed job queue
MIT License
132 stars 19 forks source link

Client does not cope with missing host in HELLO reponse #46

Open dominics opened 7 years ago

dominics commented 7 years ago

It's not well documented, but the HELLO response is not expected to always reply with an IP address for every node.

Specifically:

In this case, the Disque server does not have an IP address yet (even if you use the bind option), and the HELLO response looks like this:

127.0.0.1:7711> hello
1) (integer) 1
2) "b27f65e00cb76bb4de71dab8148f3a09d93251cd"
3) 1) "b27f65e00cb76bb4de71dab8148f3a09d93251cd"
   2) ""
   3) "7711"
   4) "1"

In this situation, the client will still consider this a valid candidate node, and will load it into the connection managers ->nodes member during revealClusterFromHello()/revealNodeFromHello(). From there, it may be selected by the priorityStrategy and put into use (because even though it doesn't have a host, it does have a low priority), causing an error.

Instead, we should filter out HELLO responses that don't have a host set (we wouldn't be able to connect to them anyway)

dominics commented 7 years ago

I've asked for clarification as to whether this is expected upstream at: https://github.com/antirez/disque/issues/201

Note that this breaks any assumption that HELLO will always return at least one valid node (it may contain none.)

dominics commented 7 years ago

This might not be such a problem because:

I think we're being saved by the fact you shouldn't be able to see a non-host HELLO response for any node but the one you originally connected to.