Update is_idle function to also consider async tasks - #901
Conversation
15fdd15 to
ca47ab4
Compare
…ain loop. Fixes ros2#747 Signed-off-by: Cornelius Krupp <cornelius@krupp.hamburg>
|
Any update or feedback regarding this PR? @wjwwood |
|
@ahcorde is there a change we get this moving? Nearly everybody I know who uses ROS2 experienced this issue at some point. |
|
It would be good to merge this, as this error is affecting many people and is quiet annoying. |
Considers non-internal active asyncio tasks in `LaunchService._is_idle()` to ensure background tasks (such as asynchronous process execution) have a chance to complete and be managed during shutdown, rather than terminating prematurely and leaving orphaned child processes. Fixes #747 Reuses test case from #901 Co-authored-by: MegaIng <26136419+MegaIng@users.noreply.github.com> Signed-off-by: William Woodall <wjwwood@google.com>
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
Sorry for the long delay in reviewing this. I needed some time to really look into it, since I deem it a pretty high risk change. I left some comments/concerns about it and would appreciate any feedback on them you guys could give, but I understand you may not have time or the issue may not be fresh for you any longer.
I opened #996 with my suggestions (preserving your tests).
| return number_of_entity_future_pairs == 0 and self.__context._event_queue.empty() | ||
| if self.event_loop is not None and self.__this_task is not None: | ||
| tasks = asyncio.all_tasks(self.event_loop) | ||
| tasks.remove(self.__this_task) |
There was a problem hiding this comment.
What if self.__this_task has completed or otherwise not present in the tasks? Maybe we should use .discard() to avoid a potential KeyError? I don't think it's actually a problem if self.__this_task isn't in the set, but it could happen I think.
There was a problem hiding this comment.
In what situation would self.__this_task be non-None and not being executed? To reach this point we must be inside an async task, and if we don't know which task we ourselves are because this task wasn't correctly being kept track of, we can't implement this check correctly.
We can switch to .discard to avoid a KeyError, but I suspect that this would just hide a programming error somewhere else.
There was a problem hiding this comment.
That's fair, I suppose it's likely that if __this_task is not in the tasks list then we're in some kind of bug state, in which case the KeyError might be appropriate. I was also thinking about thread-safety here, but ideally that shouldn't be a problem in asyncio, i.e. there's a race between checking self.__this_task and using it in the remove(), but again ideally no one would be changing that under our feet.
The pattern I've seen online is using asyncio.current_task() and storing the result of that locally until you're done with it.
We actually use that conditionally here:
But that is used to provide access to the "LaunchService" task via the getter, whereas in the place where we're using it, we actually mean "the async task we're currently in" because that's the only one we actually care about ignoring when considering the task list. It just so happens those are the same thing most of the time (perhaps all of the time).
Also, online folks tend to filter the output of all tasks against their done state, e.g. other_pending = [t for t in asyncio.all_tasks() if not t.done() and t !== current]. This also avoids the KeyError and maybe is the better approach here.
I know the existing patch has the benefit of some battle testing in production, but I also want to merge code that conceptually covers everything as well. What do you think?
There was a problem hiding this comment.
I agree that conceptually self.__this_task and current tasks are slightly different, and that changing that part is ok.
I am slightly worried about throwing away any Tasks that are marked as done() because AFAIK that doesn't mean that their return value has been used. If that return value would be used by the main loop, that could generate events, no?
There was a problem hiding this comment.
And race conditions really shouldn't be an issue as long as we don't have any await points inbetween. That's the central promise of async compared to threaded.
| tasks = asyncio.all_tasks(self.event_loop) | ||
| tasks.remove(self.__this_task) |
There was a problem hiding this comment.
You're excluding self.__this_task, but what about the process_one_event_task? I feel like you could get into a situation where that task is present, waiting for an event that will never come, but it is not excluded from the list of all tasks, there for is_idle() is also false, and you end up in a dead-lock.
Would it make more sense to track non-internal tasks in the context instead? Rather than grabbing all tasks and trying to exclude various tasks that don't matter for the shutdown condition? Maybe using context._completion_futures to track this?
There was a problem hiding this comment.
We are excluding the currently running tasks because we want to check if there is any other work that may be running.
The issue with ignoring stuff like "internal tasks" is that it may again reintroduce the error that leads to this issue - i.e. canceling stuff that may still produce events. (if there currently are no such internal tasks, they may be added in the future)
Maybe the process_one_event task should have a timeout for how long it waits for an event? It would then get rescheduled by the main loop as needed, but still giving the main loop a chance to check for being idle.
I also want to point out that this patch has been used in production, and this has not been an issue, although I am not currently deep enough in the logic to explain why it's not an issue.
There was a problem hiding this comment.
Maybe the process_one_event task should have a timeout for how long it waits for an event? It would then get rescheduled by the main loop as needed, but still giving the main loop a chance to check for being idle.
We could do that, but I dislike the idea that we have to choose a number for that timeout that either a) is very short and therefore we end up busy-waiting and wasting CPU cycles, or b) is kind of long and means that shutdown may be delayed by up to that amount of time when it could have just exited immediately (this would present on the console as a hesitation when ctrl-c'ing launch for example). Instead it would be better to not consider it in the calculation, at least in my opinion.
As for why this isn't a problem in production, I don't know either. It could be that the sequencing is already that this idle check happens after a process_one_event but before the next one is queued. My concern is that while it may not affect you in your production use case, it could be simply that the way you're using launch doesn't excite this part of the system (as in control theory).
My thinking was just that process_one_event alone should definitely not block shutdown, if present, so it should be ignored when checking for is_idle.
There was a problem hiding this comment.
My thinking was just that process_one_event alone should definitely not block shutdown
Issue is that await queue.get() could have retrieved an item, emptying the queue and then given control to the other task. This would mean process_one_event still has one event to process but is_idle would be True if we were ignoring process_one_event.
We should probably be using the task_done machinery of queue? I.e. check if the queue is empty, and all events are processed, and then I think it's safe to ignore process_one_event.
|
Tick the box to add this pull request to the merge queue (same as
|
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
Thanks for iterating. I left a few more responses, but if we don't agree to any changes, I'm inclined merge this one as-is and open follow ups.
Thanks again for the contribution!
| return number_of_entity_future_pairs == 0 and self.__context._event_queue.empty() | ||
| if self.event_loop is not None and self.__this_task is not None: | ||
| tasks = asyncio.all_tasks(self.event_loop) | ||
| tasks.remove(self.__this_task) |
There was a problem hiding this comment.
That's fair, I suppose it's likely that if __this_task is not in the tasks list then we're in some kind of bug state, in which case the KeyError might be appropriate. I was also thinking about thread-safety here, but ideally that shouldn't be a problem in asyncio, i.e. there's a race between checking self.__this_task and using it in the remove(), but again ideally no one would be changing that under our feet.
The pattern I've seen online is using asyncio.current_task() and storing the result of that locally until you're done with it.
We actually use that conditionally here:
But that is used to provide access to the "LaunchService" task via the getter, whereas in the place where we're using it, we actually mean "the async task we're currently in" because that's the only one we actually care about ignoring when considering the task list. It just so happens those are the same thing most of the time (perhaps all of the time).
Also, online folks tend to filter the output of all tasks against their done state, e.g. other_pending = [t for t in asyncio.all_tasks() if not t.done() and t !== current]. This also avoids the KeyError and maybe is the better approach here.
I know the existing patch has the benefit of some battle testing in production, but I also want to merge code that conceptually covers everything as well. What do you think?
| tasks = asyncio.all_tasks(self.event_loop) | ||
| tasks.remove(self.__this_task) |
There was a problem hiding this comment.
Maybe the process_one_event task should have a timeout for how long it waits for an event? It would then get rescheduled by the main loop as needed, but still giving the main loop a chance to check for being idle.
We could do that, but I dislike the idea that we have to choose a number for that timeout that either a) is very short and therefore we end up busy-waiting and wasting CPU cycles, or b) is kind of long and means that shutdown may be delayed by up to that amount of time when it could have just exited immediately (this would present on the console as a hesitation when ctrl-c'ing launch for example). Instead it would be better to not consider it in the calculation, at least in my opinion.
As for why this isn't a problem in production, I don't know either. It could be that the sequencing is already that this idle check happens after a process_one_event but before the next one is queued. My concern is that while it may not affect you in your production use case, it could be simply that the way you're using launch doesn't excite this part of the system (as in control theory).
My thinking was just that process_one_event alone should definitely not block shutdown, if present, so it should be ignored when checking for is_idle.
|
Tick the box to add this pull request to the merge queue (same as
|
4 participants
Description
This PR changes the
LaunchService._is_idlefunction to also consider running background tasks. This is to mitigate the error path described in this comment.The core point is that there might be background tasks running that will generate events in the future, which isn't being taken into consideration, therefore the process exits despite there still being stuff to do.
Fixes #747
Is this user-facing behavior change?
Hopefully and probably not. All old tests still work, but people might be relying on interactions that I can't predict with my limited experience with this software.
Additional Information
Another person I worked with had some issues of a test breaking in a flaky way - but it's not clear if it was caused by this change, the exact changes have changed since then and I couldn't reproduce it. Hopefully that doesn't actually relate to these changes.