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

fix xid out of range #404

Merged
merged 1 commit into from
Feb 7, 2017
Merged

fix xid out of range #404

merged 1 commit into from
Feb 7, 2017

Conversation

luofeilong
Copy link
Contributor

fix xid out of range (-2147483648 <= number <= 2147483647)

fix xid out of range (-2147483648 <= number <= 2147483647)
@@ -439,7 +439,7 @@ def _send_request(self, read_timeout, connect_timeout):
if request.type == Auth.type:
xid = AUTH_XID
else:
self._xid += 1
self._xid = (self._xid % 2147483647) + 1
Copy link
Contributor

@harlowja harlowja Dec 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can u add a comment as to why u picked this number; and a link ideally to the zookeeper docs so that if it ever changes we can know. Doesn't it also rollover to -2147483648 instead of zero (I thought this was a signed longint in java)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kazoo/protocol/connection.py, line 276
b.extend(int_struct.pack(xid))

xid += 1 in python can be bigger than 2147483648, but int_struct.pack(xid) just pack xid to int of 4 bytes, so xid must in range -2147483648~2147483647

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but doesn't this loose the negative portion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few digits in the negative portion are used, such as -1, -2
Normal communication using only positive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

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

Successfully merging this pull request may close these issues.

2 participants