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.

Sign up for GitHub

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(core): resolve race in IAsyncResult.wait() #487

Merged
merged 9 commits into from
Nov 10, 2017
Merged

fix(core): resolve race in IAsyncResult.wait() #487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
223bdfa
fix(core): resolve race in IAsyncResult.wait()
jgh-ds Nov 6, 2017
6f19cab
test(core): race in IAsyncResult.wait()
jgh-ds Nov 6, 2017
32a0cde
fix: remove unused variable
jgh-ds Nov 6, 2017
cd2191b
fix(core): add doc str, remove try/except in test_wait_race()
jgh-ds Nov 6, 2017
0cbe394
fix: remove unnecessary/dead code from test_wait_race()
jgh-ds Nov 8, 2017
0db5809
fix: made test_wait_race thread a daemon and changed timeout
jgh-ds Nov 8, 2017
2c6eec9
fix: remove extra set() call
jgh-ds Nov 8, 2017
34f9e28
fix: lengthen timeout values to be less sensitive to performance
jgh-ds Nov 8, 2017
12945fb
fix: improved comment
jgh-ds Nov 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion kazoo/handlers/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ def get_nowait(self):
def wait(self, timeout=None):
"""Block until the instance is ready."""
with self._condition:
self._condition.wait(timeout)
if not self.ready():
self._condition.wait(timeout)
return self._exception is not _NONE

def rawlink(self, callback):
Expand Down
27 changes: 27 additions & 0 deletions kazoo/tests/test_threading_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,33 @@ def wait_for_val():
eq_(lst, [True])
th.join()

def test_wait_race(self):
Copy link
Member

Choose a reason for hiding this comment

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

I thought about adding a test, but I didn't do it because a failure would cause the test code to hang forever.

Oh yeah. I forgot about that. 😄

Your test looks good to me, however it's just complicated enough that it's not immediately apparent what you're trying to test. So do you mind adding a simple docstring that explains what this is trying to test/guard against?

Maybe something like:

"""Test that there is no race condition in `IAsyncResult.wait()`.
     
Guards against https://github.com/python-zk/kazoo/issues/485 reappearing.
""""

"""Test that there is no race condition in `IAsyncResult.wait()`.

Guards against the reappearance of:
https://github.com/python-zk/kazoo/issues/485
"""
mock_handler = mock.Mock()
async_result = self._makeOne(mock_handler)

async_result.set("immediate")

cv = threading.Event()

def wait_for_val():
# NB: should not sleep
async_result.wait(20)
cv.set()
th = threading.Thread(target=wait_for_val)
th.daemon = True
th.start()

# if the wait() didn't sleep (correctly), cv will be set quickly
# if it did sleep, the cv will not be set yet and this will timeout
cv.wait(10)
eq_(cv.is_set(), True)
th.join()

def test_set_before_wait(self):
mock_handler = mock.Mock()
async_result = self._makeOne(mock_handler)
Expand Down