Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancelling asynchronous tasks has no effect? #1099

Open
haudren-woven opened this issue Mar 28, 2023 · 5 comments · May be fixed by #1377
Open

Cancelling asynchronous tasks has no effect? #1099

haudren-woven opened this issue Mar 28, 2023 · 5 comments · May be fixed by #1377
Labels
bug Something isn't working

Comments

@haudren-woven
Copy link

Bug report

Required Info:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • Binary
  • Version or commit hash:
    • Foxy
  • DDS implementation:
    • Cyclone
  • Client library (if applicable):
    • rclpy

Steps to reproduce issue

import rclpy


async def test_cancel():
    future = rclpy.task.Future()
    future.cancel()

    print(future, future.cancelled(), future.done())

    return await future


if __name__ == "__main__":
    rclpy.init()

    node = rclpy.create_node("test")
    node.create_timer(0.1, lambda: None)

    executor = rclpy.get_global_executor()
    executor.add_node(node)
    task = executor.create_task(test_cancel)

    executor.spin_until_future_complete(task)

Expected behavior

This should terminate immediately, raising a CancelledError or equivalent

Actual behavior

This runs forever

Additional information

This is related to #1098 , as I was experimenting with exceptions and asynchronous tasks. I was wondering what happens when you await a task that's cancelled. since in asyncio it raises a CancelledError. It turns out that in rclpy it does call the done callbacks, but the task is not marked as done / doesn't raise.

I'm guessing that we need to adjust this logic:

while not self._done:

To be something like:

        while not self._done and not self._cancelled:
            yield
        if self._cancelled:
            raise CancelledError()
        return self.result()

But I'd like to hear the maintainers' opinion first!

@clalancette clalancette added the bug Something isn't working label Apr 6, 2023
@weber-niklas
Copy link

weber-niklas commented Nov 3, 2024

Any updates here or plans to adjust the code to the suggested version?

@fujitatomoya
Copy link
Collaborator

@weber-niklas i think noone is working on this at this moment, besides Foxy is already E.O.L, we probably need to check if the same issue can happen with rolling and available distros. i will try to take a look in this week.

@tonygoldman
Copy link

tonygoldman commented Nov 3, 2024

It's not fixed in rolling...
In _wait_for_ready_callbacks only done tasks are filtered, cancelled tasks remain executed.

self._tasks = list(filter(lambda t_e_n: not t_e_n[0].done(), self._tasks))

Also awaiting a cancelled task blocks forever, as the __await__ method returns only when the task is done.
while not self._done:
yield

@weber-niklas i think noone is working on this at this moment, besides Foxy is already E.O.L, we probably need to check if the same issue can happen with rolling and available distros. i will try to take a look in this week.

@weber-niklas
Copy link

weber-niklas commented Nov 4, 2024

I have created another a little more complex example illustrating the problem

import rclpy
from rclpy.node import Node
from rclpy.task import Future
from rclpy.callback_groups import MutuallyExclusiveCallbackGroup


def sleep_async(node: Node, time: float) -> Future:
    """
    Sleeps for a given amount of time.

    Creates a timer with the sleep time as frequency and destroys it on the first callback.

    args:
        node: The node to sleep on. Future will be created with the nodes executor.
        time: The time to sleep in seconds
    
    returns:
        A future that will be done after the given amount of time.
    """
    future = Future(executor=node.executor)
    timer = node.create_timer(timer_period_sec=time, callback=lambda: _timer_callback(future), callback_group=MutuallyExclusiveCallbackGroup())

    def _timer_callback(future: Future):
        future.set_result(None)
        node.destroy_timer(timer)

    return future

async def sleep(self, seconds):
        for i in range(1, seconds+1):
            await sleep_async(self, 1)
            print(f"Slept for {i} seconds")


if __name__ == "__main__":
    rclpy.init()

    node = rclpy.create_node("test")
    executor = rclpy.get_global_executor()
    executor.add_node(node)
    task = executor.create_task(sleep, node, 5)

    task.cancel()
    print(f"{task.cancelled()=}")
    executor.spin_until_future_complete(task)

    print(f"{task.done()=}")
    print(f"{task.cancelled()=}")

Expected output:

task.cancelled()=True
task.done()=False
task.cancelled()=True

Actual output:

task.cancelled()=True
Slept for 1 seconds
Slept for 2 seconds
Slept for 3 seconds
Slept for 4 seconds
Slept for 5 seconds
task.done()=True
task.cancelled()=False

The task does not cancel, runs till completed and then overrides the cancellation when setting the result.

fujitatomoya added a commit to fujitatomoya/ros2_test_prover that referenced this issue Nov 7, 2024
@fujitatomoya fujitatomoya linked a pull request Nov 8, 2024 that will close this issue
@fujitatomoya
Copy link
Collaborator

@tonygoldman @weber-niklas if you can share the feedback to #1377, that would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants