fix(core): resolve race in IAsyncResult.wait() #487
Conversation
|
I thought about adding a test, but I didn't do it because a failure would cause the test code to hang forever. However, it would be very simple to do. Should I add one? |
|
I didn't mean to close it. |
| def wait_for_val(): | ||
| try: | ||
| # NB: should not sleep | ||
| val = async_result.wait(10) |
There was a problem hiding this comment.
local variable 'val' is assigned to but never used
|
Updated jgh-ds:master with a new test case for the issue: test_wait_race() |
|
It should be ready to merge now, including a test case. Thanks. |
| @@ -200,6 +200,28 @@ def wait_for_val(): | |||
| eq_(lst, [True]) | |||
| th.join() | |||
|
|
|||
| def test_wait_race(self): | |||
There was a problem hiding this comment.
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.
""""
| async_result.wait(10) | ||
| cv.set() | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
why is this needed? what would cause an ImportError?
There was a problem hiding this comment.
Copied from test_wait(). I'll remove it.
|
Added the doc string and removed the unnecessary try/except from the test case. Also verified that failing the test case will not hang the testing process. |
|
I'm going to go ahead and merge. The test is the only part I'm slightly unsure of, but I am confident the race condition exists and your fix looks good. |
|
I've determined that the part of the test which is: is not needed (but is harmless). I was thinking that it would be necessary in the failure case, but the assertion failure from eq_(_) keeps it from being run in that case anyway. Since the thread has a timeout anyway, it cannot hang the testing process. Would it be better to remove it? |
|
Yes, please remove it. No one later will realize it can be removed. Re: the merge, I accidentally clicked the wrong button, then had to wait til tests passed... once you make this change and tests pass, I will merge. |
|
The dead code in the test is removed now. |
|
Apparently the removed code is needed in Python 3.5 and 3.6 to avoid hanging. |
|
The test failures are not on the new test case. The actual IAsyncResult.wait() code is the same as the first commit, where all the tests pass. Could other test cases depend on the old (broken) behavior? The latest errors are on test_race_condition_new_partitioner_steals_the_lock |
|
I've verified that the failing test case in test_race_condition_new_partitioner_steals_the_lock does not use the modified IAsyncResult.wait() code. |
|
It should be good to go now. |
closes #485