Fix. Set correct peer status on HELLO #219

Merged
merged 1 commit into from Jul 24, 2014

Projects

None yet

3 participants

@moteus
Member
moteus commented Jul 24, 2014

Suppose we get HELLO with group CHAT and status 1.
If we set peer status and then call zyre_node_join_peer_group then
peer status become to 2.
So we should set status after we call zyre_node_join_peer_group.

@moteus moteus Fix. Set correct peer status on HELLO
Suppose we get HELLO with group `CHAT` and status `1`.
If we set peer status and then call `zyre_node_join_peer_group` then
peer status become to `2`.
So we should set status after we call `zyre_node_join_peer_group`.
4e224cc
@keent keent merged commit d3fb0ec into zeromq:master Jul 24, 2014
@hintjens
Member

I don't see how zyre_node_join_peer_group() modifies the peer's status. Moving this line around makes no difference as far as I can tell. Did you have a test case?

@moteus
Member
moteus commented Jul 25, 2014

zyre_node_join_peer_group calls zyre_group_join which increment peer's status

@hintjens
Member

Ah, nice catch! I'd missed that. Thanks.

On Fri, Jul 25, 2014 at 7:36 AM, Alexey Melnichuk notifications@github.com
wrote:

zyre_node_join_peer_group calls zyre_group_join
https://github.com/zeromq/zyre/blob/master/src/zyre_node.c#L522 which increment
peer's status
https://github.com/zeromq/zyre/blob/master/src/zyre_group.c#L94


Reply to this email directly or view it on GitHub
#219 (comment).

@hintjens
Member

In fact since we're joining the peer to all its groups, its status (as we
calculate it) should match what's in HELLO, right? Rather than overwrite
it, let's assert (or at least check) they're the same.

On Fri, Jul 25, 2014 at 10:18 AM, Pieter Hintjens ph@imatix.com wrote:

Ah, nice catch! I'd missed that. Thanks.

On Fri, Jul 25, 2014 at 7:36 AM, Alexey Melnichuk <
notifications@github.com> wrote:

zyre_node_join_peer_group calls zyre_group_join
https://github.com/zeromq/zyre/blob/master/src/zyre_node.c#L522 which increment
peer's status
https://github.com/zeromq/zyre/blob/master/src/zyre_group.c#L94


Reply to this email directly or view it on GitHub
#219 (comment).

@moteus
Member
moteus commented Jul 25, 2014

I do not think so.
Basic example.
Node can JOIN->LEAVE->... after that in network appears new node and we send HELLO.
Our status is 2 but there no groups in HELLO.
Also I do not think it is good idea to assert if we get wrong message from some client.
This is public protocol and now one nede in network can crash all nodes.
We can Immediately reconnect, delay reconnect or ban such nodes and behaviour may depend from application or described in RFC.

@hintjens
Member

OK, good points.

On Fri, Jul 25, 2014 at 10:34 AM, Alexey Melnichuk <notifications@github.com

wrote:

I do not think so.
Basic example.
Node can JOIN->LEAVE->... after that in network appears new node and we
send HELLO.
Our status is 2 but there no groups in HELLO.
Also I do not think it is good idea to assert if we get wrong message from
some client.
This is public protocol and now one nede in network can crash all nodes.
We can Immediately reconnect, delay reconnect or ban such nodes and
behaviour may depend from application or described in RFC.


Reply to this email directly or view it on GitHub
#219 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment