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

fix timers in task #993

Open
wants to merge 1 commit into
base: humble
Choose a base branch
from
Open

Conversation

koonpeng
Copy link

Fixes #985.

I am not familiar with the executor logic in rclpy, so this is a hacky fix I did. Instead of passing a new timeout each tick, I added self._spin_until_future_complete_timer and self._spin_until_future_complete_timeout to wake the executor when the timeout is reached. Also added test_timer_in_task.py to test the new behavior.


Looking at the code of spin_until_future_complete, it behaves differently depending on if a timeout is provided.

        if timeout_sec is None or timeout_sec < 0:
            while self._context.ok() and not future.done() and not self._is_shutdown:
                self.spin_once_until_future_complete(future, timeout_sec)
        else:
            start = time.monotonic()
            end = start + timeout_sec
            timeout_left = timeout_sec

            while self._context.ok() and not future.done() and not self._is_shutdown:
                self.spin_once_until_future_complete(future, timeout_left)
                now = time.monotonic()

                if now >= end:
                    return

                timeout_left = end - now

This all looks good but furthur investigation of wait_for_ready_callbacks (which is eventually called later down the chain),

        while True:
            if self._cb_iter is None or self._last_args != args or self._last_kwargs != kwargs:
                # Create a new generator
                self._last_args = args
                self._last_kwargs = kwargs
                self._cb_iter = self._wait_for_ready_callbacks(*args, **kwargs)

            try:
                return next(self._cb_iter)
            except StopIteration:
                # Generator ran out of work
                self._cb_iter = None

It creates a new generator whenever _last_args or _last_kwargs is different, the "timeout" path of spin_until_future_complete does exactly that, each spin_once_until_future_complete is passed a different timeout.

My guess is that somehow creating new generators every "tick" causes new "work" to be created, and executing a "work" causes a new generator to be created, resulting in a infinite list of pending "work", which causes the actual task to never be executed.

Signed-off-by: Teo Koon Peng <[email protected]>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding timer based on timeout argument looks okay to avoid to keep creating generator, but i cannot really say what approach is reasonable.

@sloretz @ivanpauno could you share your thoughts?

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's something weird somewhere else, this doesn't look like the right fix.

Based on your comments in #985, I think we should be fixing what wait_for_ready_callbacks()/_wait_for_ready_callbacks() are doing.
I haven't exactly figured out what changes are needed, I will try to look again soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants