Skip to content

Problem: No wait when getting value from queue#2423

Merged
vrde merged 2 commits intobigchaindb:masterfrom
kansi:iss/random-event-test-failure
Aug 8, 2018
Merged

Problem: No wait when getting value from queue#2423
vrde merged 2 commits intobigchaindb:masterfrom
kansi:iss/random-event-test-failure

Conversation

@kansi
Copy link
Copy Markdown
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 muawiakh and ttmc July 31, 2018 09:47
@codecov-io
Copy link
Copy Markdown

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
Copy Markdown
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.


try:
self.started_queue.get_nowait()
self.started_queue.get(timeout=0.01)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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