search

python-zk / kazoo

Kazoo is a high-level Python library that makes it easier to use Apache Zookeeper.
https://kazoo.readthedocs.io
Apache License 2.0
1.3k stars 387 forks source link

fix: ensure timeout argument is nonnegative #534

Closed cdonati closed 6 years ago

cdonati commented 6 years ago

Previously, a gap between calls to time.time() could lead to a situation where the current time was less than end during the while condition, but it was greater than end when assigning a value to timeout_at.

This is the same issue describe in https://github.com/python-zk/kazoo/pull/518, but the fix is slightly different. It avoids a redundant call to time.time(). I've also included a unit test that exhibits the problem when run without the fix.

cdonati commented 6 years ago

I didn't notice the previous pull request until I was submitting this one, so I added the last commit to address the concern about 0 being passed to socket.settimeout.

@StephenSorriaux Your suggestion about just passing timeout along makes sense. The socket timeout should probably not be influenced by the amount it takes to establish the connection. However, in the interest of keeping the changes here to a minimum, I'll let a different pull request address that behavior.

cdonati commented 6 years ago

@StephenSorriaux Thanks for the review. Originally, I wasn't sure if (core) was applicable here for the scope, but that makes sense now.

In addition to the tests you suggested, I also added a couple tests for nonpositive timeout values.

Let me know if the amended commits address all of your comments.

cdonati commented 6 years ago

@StephenSorriaux Your changes look good to me. Would you prefer I use the "Apply suggestion" feature on all of them or should I push the changes you suggested as a single additional commit?

StephenSorriaux commented 6 years ago

@cdonati I think it is better that you push the changes, I'm not sure how many commits will be created using the "Apply suggestion" feature. Just keep only 1 commit on your branch and it would be perfect.

cdonati commented 6 years ago

@StephenSorriaux I just saw your comment about a single commit. I squashed the commits accordingly.

StephenSorriaux commented 6 years ago

@cdonati Thanks again for your PR, I'm merging it to master.