Bugfix: executor doesn't propagate exception from task that awaited a future#1643
Conversation
Signed-off-by: Nadav Elkabets <elnadav12@gmail.com>
Signed-off-by: Nadav Elkabets <elnadav12@gmail.com>
Signed-off-by: Nadav Elkabets <elnadav12@gmail.com>
Signed-off-by: Nadav Elkabets <elnadav12@gmail.com>
Signed-off-by: Nadav Elkabets <elnadav12@gmail.com>
fujitatomoya
left a comment
There was a problem hiding this comment.
i think that the approach of preserving task identity rather than wrapping in a new task is the right call. almost lgtm, with just one minor suggestion.
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI.
|
Pulls: #1643 |
|
@fujitatomoya could you run windows again? |
|
@Mergifyio backport kilted jazzy humble |
✅ Backports have been createdDetails
Cherry-pick of aac0ebb has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Cherry-pick of aac0ebb has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
… future (#1643) * Schedule the original task when task awaits a future Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add MultiThreadedExecutor to test Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add tests for awaiting a done future and task cancellation during await Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Removed unused variable Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> --------- Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> (cherry picked from commit aac0ebb)
… future (#1643) * Schedule the original task when task awaits a future Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add MultiThreadedExecutor to test Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add tests for awaiting a done future and task cancellation during await Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Removed unused variable Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> --------- Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> (cherry picked from commit aac0ebb) # Conflicts: # rclpy/rclpy/task.py # rclpy/test/test_executor.py
… future (#1643) * Schedule the original task when task awaits a future Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add MultiThreadedExecutor to test Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Add tests for awaiting a done future and task cancellation during await Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> * Removed unused variable Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> --------- Signed-off-by: Nadav Elkabets <elnadav12@gmail.com> (cherry picked from commit aac0ebb) # Conflicts: # rclpy/rclpy/task.py # rclpy/test/test_executor.py
Fixes #1642
Changes
A task now registers itself on the awaited future via new
Future._add_waiting_task.The executor then dispatches the same
Taskobject and sees its exception throughhandler.exception()as intended.Future._callbacksnow holdsUnion[Callable, Task]_schedule_or_invoke_done_callbackshandlesTaskentries throughexecutor._call_task_in_next_spin._wait_for_ready_callbacksskips tasks that were cancelled or completed between being queued and being popped.warnings.warnfires if a waitingTaskis dropped because the executor weakref could not be resolved.