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

Problem: No wait when getting value from queue #2423

Merged
merged 2 commits into from
Aug 8, 2018

Conversation

kansi
Copy link
Contributor

@kansi kansi commented Jul 31, 2018

Solution: Random failure of test tests/test_events.py::test_event_handler_raises_when_called_after_start seems to be because of get_nowait which returns way to quickly even when the queue has data i.e. the test expect the queue to have the message 'STARTED' which should be returned when .get_nowait() is called. But since get_nowait() returns without waiting which causes a queue Empty exception rather than Runtime exception.

Solution: Random failure of test
`tests/test_events.py::test_event_handler_raises_when_called_after_start` seem
to be because of `get_nowait` which returns way to quickly even when the queue
has data
@kansi kansi requested review from ttmc and muawiakh July 31, 2018 09:47
@codecov-io
Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #2423 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2423      +/-   ##
==========================================
+ Coverage   88.43%   88.48%   +0.04%     
==========================================
  Files          39       39              
  Lines        2275     2275              
==========================================
+ Hits         2012     2013       +1     
+ Misses        263      262       -1

Copy link
Contributor

@ttmc ttmc left a comment

Choose a reason for hiding this comment

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

I don't fully understand what's going on here, but the description is much better now, so it should help others understand the reasoning in the future.

@@ -67,7 +67,7 @@ def get_subscriber_queue(self, event_types=None):
"""

try:
self.started_queue.get_nowait()
self.started_queue.get(timeout=0.01)
Copy link
Contributor

Choose a reason for hiding this comment

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

A bigger timeout (say, a second) won't hurt - a successful build will take the same amount of time anyway while if there is an issue, waiting a second is not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't know what would be an appropriate value of this timeout i.e. 10ms, 100ms, 1sec? But I do understand the having no timeout at all would lead us back to the situation wherein the test failed randomly. Also one must keep in mind that while the timeout will help the test case it will also impact the code performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since CI often fails for us here it makes sense to stay on the safe side and set a second. It only impacts the performance of the unsuccessful case. Do you expect failed retrievals to happen a lot? I suppose, we should not expect them to happen often so it's ok to wait for a second if it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you expect failed retrievals to happen a lot?

The only place where get_subscriber_queue is called in the code base is here. So it seems that it might not effect much.

@kansi kansi requested a review from ldmberman July 31, 2018 10:26
@vrde vrde merged commit 54b81d3 into bigchaindb:master Aug 8, 2018
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.

5 participants