Simplify implementation of CallbackQueue::callOne()#1
Closed
meyerj wants to merge 2 commits intocwecht:fix_subscription_busy_wait-melodicfrom
Closed
Simplify implementation of CallbackQueue::callOne()#1meyerj wants to merge 2 commits intocwecht:fix_subscription_busy_wait-melodicfrom
meyerj wants to merge 2 commits intocwecht:fix_subscription_busy_wait-melodicfrom
Conversation
f7308e6 to
d84ecc1
Compare
3db1254 to
52dfdb1
Compare
749246e to
1ae22ec
Compare
Replace duplicate wait_for() call in CallbackQueue::callOne() with a loop and a call to wait_until().
d84ecc1 to
3a98900
Compare
meyerj
commented
Jul 31, 2020
…ely if timeout.isZero() ... and if none of the other result conditions holds (i.e. the queue is Empty or Disabled).
|
I created ros#2014 to run CI for this change and be able to merge it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was working on an ABI-compatible solution for ros#1545 because we were also affected by this problem. Only afterwards I found your PR ros#1684 based on what @peci1 suggested in ros#1608. I came up with a similar solution and merged it at the end. Maybe it would be good to explicitly reference ros#1684 in ros#1608 and close the latter?
I suggest to replace the duplicate
wait_for()call inCallbackQueue::callOne()with a loop and a single call towait_until(). There is also no need to calculate the remaining duration of the timeout explicitly.The behavior is slightly different:
callOne(timeout)only returnsTryAgainif the timeout expired and none of the callbacks in the queue was ready. If one of the callbacks gets ready within the given timeout, it will be called without returning to the caller. That does not make a significant difference at the end if the caller invokescallOne()again immediately like the spinner does.I needed theBOOST_THREAD_HAS_CONDATTR_SET_CLOCK_MONOTONICto be defined because I am currently testing with Boost 1.58 in Ubuntu 16.04 (custom melodic build) (ros#1684 (comment)). If it gets removed, theBOOST_VERSIONcheck in line 44-50 should be removed, too.I can confirm that when building in debug mode without compiler optimizations, the unit test fails without also applying ros#1651.