Fix spin_once_until_future_complete to quit when the future finishes. #1143
Conversation
This makes it match the function name much more closely. While we are in here, also fix a bug where the multi-threaded version would not quit immediately after the future completes. And also add tests for both of these situations. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
| future.result() # re-raise any exceptions | ||
|
|
||
| def spin_once(self, timeout_sec: Optional[float] = None) -> None: | ||
| self._spin_once_impl(timeout_sec) |
There was a problem hiding this comment.
What, if anything, should be done with self._futures here? Is it intended that the futures are held until the next call?
There was a problem hiding this comment.
I'm not sure I totally understand the question, but let me explain what I understand happens to the futures in both spin_once and spin_once_until_future_complete.
When you are calling spin_once, then what happens is that our wait_condition for _spin_once_impl is the default lambda: False. That means that that condition will never cause the work loop (in _wait_for_ready_callbacks) to return early, and so that function will always find one piece of work (assuming it doesn't time out or shutdown). When it does find that piece of work, it executes it immediately in _spin_once_impl. After that we add it to the _futures list to check on whether it is done. We then also look at any other futures that were pending, and if those are done we execute the done handlers for those and remove them from the list.
When you are calling spin_once_until_future_complete, the logic is largely the same except that our wait_condition is the done callback on the future. So if the future finishes before we find other work to do, that can cause wait_for_ready_callbacks to return early. If that is the case, then it actually raises ConditionReachedException, because we have already called the done callback. And thus it is never added to _futures.
Does that make sense to you? And does that answer your question at all?
There was a problem hiding this comment.
@clalancette thanks for the PR. I just have a question before lgtm.
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Because we are executing the handler directly, we don't need to store it and check for futures later. We can just check the result immediately. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
|
CI:
|
delete-merged-branch
bot
deleted the
clalancette/spin-once-until-future-complete
branch
This makes it match the function name much more closely. While we are in here, also fix a bug where the multi-threaded version would not quit immediately after the future completes. And also add tests for both of these situations.
This should fix #989