Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dominics opened this issue May 30, 2017 · 2 comments
Open

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

dominics opened this issue May 30, 2017 · 2 comments

Comments

@dominics
Copy link
Contributor

dominics commented May 30, 2017

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

Specifically:

  • Shut down any running Disque instance
  • Clear any existing nodes.conf
  • Start a single-node cluster of Disque (must never have had CLUSTER MEET called on it)
  • Connect and call HELLO

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
Copy link
Contributor Author

dominics commented Jun 8, 2017

I've asked for clarification as to whether this is expected upstream at: antirez/disque#201

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

@dominics
Copy link
Contributor Author

dominics commented Jun 8, 2017

This might not be such a problem because:

  • revealNodeFromHello will defer to an existing node instance
  • if you directly connect to the node with no host in the HELLO, you would have done so via Credentials, and the node would have been created through findAvailableConnection() -> createNode(), so should have the correct details

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant