Check if Task(Future) is canceled. #1377

fujitatomoya · 2024-11-08T01:11:38Z

Signed-off-by: Tomoya Fujita <[email protected]>

rclpy/test/test_executor.py

rclpy/rclpy/task.py

nadavelkabets · 2024-11-08T19:06:02Z

Please backport this bugfix into humble/jazzy.
Cancelling tasks is a crucial feature of any event loop, and for a project of mine I had to come up with complex and incomplete solutions to make that work.

weber-niklas · 2024-12-20T20:30:09Z

Hey,
any updates here?
I think @fujitatomoya was pretty close to make it work.
Maybe if we go with @nadavelkabets suggestion number 2:

def cancel(self):
	if not self._done and iscoroutine(self._handle):
		  self._handle.close()
	super().cancel()

We can at least fix the bug at hand and concentrate on improving on the functionality after?
I am currently having a pretty hard time using Tasks since a bunch of it is broken. Would love to get a fix out which makes the feature usable. Thanks for the work you guys put in <3

fujitatomoya · 2024-12-20T23:45:40Z

@nadavelkabets @weber-niklas sorry guys for being late to get back to this, i will try to allocate some time for this in next week.

nadavelkabets · 2025-01-04T10:37:59Z

@nadavelkabets @weber-niklas sorry guys for being late to get back to this, i will try to allocate some time for this in next week.

I created PR #1392 to fix the unawaited coroutine issue based on my first suggestion. It's not yet complete since we need to figure out how to handle the CancelledException in a way that won't crash the executor.

I believe we should first go with a solution that we can backport into jazzy.
Do you think suggestion 1 is viable for a backport, or should we start with suggestion 2 and add suggestion 1 for the next version?

fujitatomoya · 2025-01-05T01:47:10Z

@nadavelkabets again thank you very much for helping this one moving forward 👍

i have already #1394 merged in this PR.

I believe we should first go with a solution that we can backport into jazzy.

agree, this is also broken in jazzy and humble.

Do you think suggestion 1 is viable for a backport, or should we start with suggestion 2 and add suggestion 1 for the next version?

IMO, we can go with suggestion 2 (coroutine close, that is currently implemented in this PR) to avoid surprising the application with new exception generated by task cancel. that said it does not break the user space, user application can have the fix and also no user code change is required.

And then probably we can move on to #1392 for rolling branch. i think having the capability for clean-up in the user callback makes sense for canceling tasks.

It's not yet complete since we need to figure out how to handle the CancelledException in a way that won't crash the executor.

i will look into this for sure and leave some comments.

* Add FutureState Signed-off-by: Nadav Elkabets <[email protected]> * Close canceled coroutine Signed-off-by: Nadav Elkabets <[email protected]> * Fixed behavior in test Signed-off-by: Nadav Elkabets <[email protected]> --------- Signed-off-by: Nadav Elkabets <[email protected]> Signed-off-by: Tomoya Fujita <[email protected]>

Signed-off-by: Tomoya Fujita <[email protected]>

nadavelkabets · 2025-01-11T14:55:28Z

@fujitatomoya please review #1396, it includes some fixes I forgot to add in my last PR.
Is there anything I can do to help expedite this PR so we can review and merge it in the near future?

fujitatomoya · 2025-01-11T17:35:05Z

@nadavelkabets thank for the PR against this one, can you also take a look at the unresolved comment here? that is directly related to #1396.

rclpy/rclpy/task.py

nadavelkabets · 2025-01-13T09:00:36Z

I created a new PR #1397 that should close your comments @fujitatomoya.

* Remove redundant coro.close Signed-off-by: nadav <[email protected]> * Only finished future is done Signed-off-by: nadav <[email protected]> * Add function _pending and fix checks Signed-off-by: = <[email protected]> * Replace check in done from pending to finished Signed-off-by: = <[email protected]> * Adapt test to new behavior Signed-off-by: = <[email protected]> * Add tests Signed-off-by: = <[email protected]> * Make changes within active task mutex Signed-off-by: = <[email protected]> --------- Signed-off-by: nadav <[email protected]> Signed-off-by: = <[email protected]>

rclpy/rclpy/task.py

…sts accordingly. Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya · 2025-01-13T19:29:32Z

@nadavelkabets can you double-check 6cd294b just in case? that just removes some flake8 errors, and removed new exception that you introduced with previous commit. do you think this is okay?

rclpy/rclpy/task.py

nadavelkabets · 2025-01-13T20:08:21Z

@nadavelkabets can you double-check 6cd294b just in case? that just removes some flake8 errors, and removed new exception that you introduced with previous commit. do you think this is okay?

Looks good to me.
I think this PR is ready to move forward.

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya · 2025-01-13T21:14:05Z

Pulls: #1377
Gist: https://gist.githubusercontent.com/fujitatomoya/640f3a1c4e6f8072674dbe10b4778e7a/raw/b5a624de3bc2d1b1582cc1d000d22c2618a58681/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15053

Linux
Linux-aarch64
Linux-rhel
Windows

rclpy/rclpy/executors.py

fujitatomoya · 2025-01-14T04:33:39Z

@tfoote thanks for the review, i will address them.

besides that, CI RHEL and Windows are not happy with ImportError: cannot import name 'StrEnum' from 'enum'

rclpy/rclpy/task.py

nadavelkabets · 2025-01-14T07:28:37Z

besides that, CI RHEL and Windows are not happy with ImportError: cannot import name 'StrEnum' from 'enum'

Good catch. StrEnum was only added in python3.11.
I added suggestions for a fix.
@fujitatomoya should I open a new PR?

fujitatomoya · 2025-01-14T18:19:57Z

@nadavelkabets no that is fine, i can patch them today.

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya · 2025-01-14T19:15:19Z

@tfoote @nadavelkabets addressed review comments, can you check fc3dd67?

fujitatomoya · 2025-01-14T19:16:08Z

Pulls: #1377
Gist: https://gist.githubusercontent.com/fujitatomoya/bed30f370697213781f69ecce74a03e0/raw/b5a624de3bc2d1b1582cc1d000d22c2618a58681/ros2.repos
BUILD args: --packages-above-and-dependencies rclpy
TEST args: --packages-above rclpy
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15055

Linux
Linux-aarch64
Linux-rhel
Windows

tfoote

I have a few suggestions to make sure we have better test coverage, but it looks good to me otherwise assuming CI passes.

rclpy/test/test_task.py

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya · 2025-01-14T20:42:04Z

@nadavelkabets @weber-niklas @haudren-woven @tonygoldman

just a quick heads up. thank you very much for being patient on this PR.

this PR is almost ready to merge in rolling. once i confirm the all CI goes green, i will merge this to rolling branch 1st. since the original issue created on released distro (actually Foxy), and this is really nice to fix to downstream distros Jazzy and Humble, i will try to backport this after a couple of weeks soak time in rolling.

and special thanks to @nadavelkabets for helping me to push forward this fix 👍

nadavelkabets · 2025-01-14T20:51:56Z

Awesome! It’s great news for any ‘async ros’ user!
@fujitatomoya thank you for dedicating the time and effort to this.
Honestly I’ve been waiting for this fix for the last two years…
It’s never too late😃

fujitatomoya · 2025-01-14T23:32:15Z

windows is unstable, but i do not think failures are related to this PR.

retry CI (windows only):

Windows

fujitatomoya · 2025-01-15T16:26:36Z

windows CI is green, merging.

fujitatomoya · 2025-01-17T18:46:11Z

@Mergifyio backport humble jazzy

mergify · 2025-01-17T18:46:23Z

backport humble jazzy

✅ Backports have been created

#1403 Check if Task(Future) is canceled. (backport #1377) has been created for branch humble but encountered conflicts
#1404 Check if Task(Future) is canceled. (backport #1377) has been created for branch jazzy but encountered conflicts

* Check if Task(Future) is canceled. Signed-off-by: Tomoya Fujita <[email protected]> * Close cancelled coroutine (#1394) * Add FutureState Signed-off-by: Nadav Elkabets <[email protected]> * Close canceled coroutine Signed-off-by: Nadav Elkabets <[email protected]> * Fixed behavior in test Signed-off-by: Nadav Elkabets <[email protected]> --------- Signed-off-by: Nadav Elkabets <[email protected]> Signed-off-by: Tomoya Fujita <[email protected]> * address flake8 and pep257 failures. Signed-off-by: Tomoya Fujita <[email protected]> * Cancelled future is not done (#1397) * Remove redundant coro.close Signed-off-by: nadav <[email protected]> * Only finished future is done Signed-off-by: nadav <[email protected]> * Add function _pending and fix checks Signed-off-by: = <[email protected]> * Replace check in done from pending to finished Signed-off-by: = <[email protected]> * Adapt test to new behavior Signed-off-by: = <[email protected]> * Add tests Signed-off-by: = <[email protected]> * Make changes within active task mutex Signed-off-by: = <[email protected]> --------- Signed-off-by: nadav <[email protected]> Signed-off-by: = <[email protected]> * keep the consistent behavior to avoid exception, and adjusted some tests accordingly. Signed-off-by: Tomoya Fujita <[email protected]> * revert doc section to raise the exception. Signed-off-by: Tomoya Fujita <[email protected]> * remove StrEnum and put logical operator in the beginning of line. Signed-off-by: Tomoya Fujita <[email protected]> * add more test to check Task state. Signed-off-by: Tomoya Fujita <[email protected]> --------- Signed-off-by: Tomoya Fujita <[email protected]> Signed-off-by: Nadav Elkabets <[email protected]> Signed-off-by: nadav <[email protected]> Signed-off-by: = <[email protected]> Co-authored-by: Nadav Elkabets <[email protected]> Co-authored-by: Nadav Elkabets <[email protected]> (cherry picked from commit 9a144bf) # Conflicts: # rclpy/rclpy/executors.py # rclpy/rclpy/task.py # rclpy/test/test_executor.py

Check if Task(Future) is canceled.

d2e647c

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya requested review from sloretz and adityapande-1995 as code owners November 8, 2024 01:11

fujitatomoya self-assigned this Nov 8, 2024

fujitatomoya commented Nov 8, 2024

View reviewed changes

rclpy/test/test_executor.py Show resolved Hide resolved

fujitatomoya mentioned this pull request Nov 8, 2024

Cancelling asynchronous tasks has no effect? #1099

Closed

nadavelkabets reviewed Nov 8, 2024

View reviewed changes

rclpy/rclpy/task.py Outdated Show resolved Hide resolved

nadavelkabets mentioned this pull request Jan 4, 2025

Throw CancelledException into cancelled tasks #1392

Closed

nadavelkabets mentioned this pull request Jan 4, 2025

Close cancelled coroutine #1394

Merged

nadavelkabets and others added 2 commits January 4, 2025 18:00

address flake8 and pep257 failures.

59000b6

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya force-pushed the fujitatomoya/check-task-canceled branch from c3107d6 to 59000b6 Compare January 5, 2025 02:00

fujitatomoya mentioned this pull request Jan 11, 2025

Fix redundant checks for a canceled future #1396

Closed

fujitatomoya commented Jan 11, 2025

View reviewed changes

rclpy/rclpy/task.py Outdated Show resolved Hide resolved

fujitatomoya commented Jan 13, 2025

View reviewed changes

rclpy/rclpy/task.py Outdated Show resolved Hide resolved

keep the consistent behavior to avoid exception, and adjusted some te…

6cd294b

…sts accordingly. Signed-off-by: Tomoya Fujita <[email protected]>

nadavelkabets reviewed Jan 13, 2025

View reviewed changes

rclpy/rclpy/task.py Outdated Show resolved Hide resolved

revert doc section to raise the exception.

628a9ee

Signed-off-by: Tomoya Fujita <[email protected]>

tfoote reviewed Jan 13, 2025

View reviewed changes

rclpy/rclpy/executors.py Show resolved Hide resolved

tfoote reviewed Jan 13, 2025

View reviewed changes

rclpy/rclpy/executors.py Outdated Show resolved Hide resolved

nadavelkabets reviewed Jan 14, 2025

View reviewed changes

rclpy/rclpy/task.py Outdated Show resolved Hide resolved

remove StrEnum and put logical operator in the beginning of line.

fc3dd67

Signed-off-by: Tomoya Fujita <[email protected]>

tfoote approved these changes Jan 14, 2025

View reviewed changes

rclpy/test/test_task.py Show resolved Hide resolved

rclpy/test/test_task.py Show resolved Hide resolved

rclpy/test/test_task.py Show resolved Hide resolved

rclpy/test/test_task.py Show resolved Hide resolved

rclpy/test/test_task.py Show resolved Hide resolved

add more test to check Task state.

8d4eefb

Signed-off-by: Tomoya Fujita <[email protected]>

fujitatomoya merged commit 9a144bf into rolling Jan 15, 2025
3 checks passed

This was referenced Jan 17, 2025

Check if Task(Future) is canceled. (backport #1377) #1403

Open

Check if Task(Future) is canceled. (backport #1377) #1404

Open

tfoote deleted the fujitatomoya/check-task-canceled branch January 19, 2025 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if Task(Future) is canceled. #1377

Check if Task(Future) is canceled. #1377

fujitatomoya commented Nov 8, 2024

nadavelkabets commented Nov 8, 2024

weber-niklas commented Dec 20, 2024

fujitatomoya commented Dec 20, 2024

nadavelkabets commented Jan 4, 2025 •

edited

Loading

fujitatomoya commented Jan 5, 2025

nadavelkabets commented Jan 11, 2025 •

edited

Loading

fujitatomoya commented Jan 11, 2025

nadavelkabets commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

nadavelkabets commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

fujitatomoya commented Jan 14, 2025

nadavelkabets commented Jan 14, 2025 •

edited

Loading

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025 •

edited

Loading

tfoote left a comment

fujitatomoya commented Jan 14, 2025

nadavelkabets commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 15, 2025

fujitatomoya commented Jan 17, 2025

mergify bot commented Jan 17, 2025 •

edited

Loading

Check if Task(Future) is canceled. #1377

Check if Task(Future) is canceled. #1377

Conversation

fujitatomoya commented Nov 8, 2024

nadavelkabets commented Nov 8, 2024

weber-niklas commented Dec 20, 2024

fujitatomoya commented Dec 20, 2024

nadavelkabets commented Jan 4, 2025 • edited Loading

fujitatomoya commented Jan 5, 2025

nadavelkabets commented Jan 11, 2025 • edited Loading

fujitatomoya commented Jan 11, 2025

nadavelkabets commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

nadavelkabets commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

fujitatomoya commented Jan 14, 2025

nadavelkabets commented Jan 14, 2025 • edited Loading

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025 • edited Loading

tfoote left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 14, 2025

nadavelkabets commented Jan 14, 2025

fujitatomoya commented Jan 14, 2025

fujitatomoya commented Jan 15, 2025

fujitatomoya commented Jan 17, 2025

mergify bot commented Jan 17, 2025 • edited Loading

✅ Backports have been created

nadavelkabets commented Jan 4, 2025 •

edited

Loading

nadavelkabets commented Jan 11, 2025 •

edited

Loading

nadavelkabets commented Jan 14, 2025 •

edited

Loading

fujitatomoya commented Jan 14, 2025 •

edited

Loading

mergify bot commented Jan 17, 2025 •

edited

Loading