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

Retry after tick #723

Closed
wants to merge 2 commits into from

Conversation

renan028
Copy link
Contributor

Description

The RetryUntilSuccessful decorator retries the child on the SAME tick, thus it behaves like an independent tree, instead of forwarding the tick from the root. This behavior was unexpected for me.
For instance, consider the example below:

image

  1. The condition succeeds once and then, on a second tick, it fails.
  2. Assume the action below the retry always fails.

I would expect the tree to exit on the second tick with FAILURE, because the condition would fail. However, the tree is stuck forever on the decorator (and blocked there).

I have added a unittest with the behavior I was expecting.

However, maybe this behavior change could affect a lot of users. so maybe we can discuss solutions?

thanks!

@facontidavide
Copy link
Collaborator

I am afraid that this is a problem with no easy solution.
The only alternative is to create a different Retry with he behavior you are proposing ("RetryAsync").

See for instance the difference between Sequence and SequenceAsync

In fact, if i accept this, this other code will be broken instead:

image

@corot
Copy link

corot commented Dec 21, 2023

In fact, if i accept this, this other code will be broken instead:

To my understanding of BT, this PR implements the proper logic for a retry. I was very surprised when I saw that it was not returning RUNNING on failure. That means the decorator is doing ticking by itself instead of just propagating the tick from the root as decorators are supposed to do.

The case you mention will get broken, true, but that's the result of how ReactiveSequence is implemented: it only halts already executed nodes instead of the following ones. If you do the later, there's no problem on having several async nodes in a reactive sequence (so you can disable this check). Moreover, we only need to halt already executed nodes because this condition prevents re-starting succeeded nodes; changing it from

  if (initial_status == NodeStatus::IDLE)

to

if (initial_status != NodeStatus::RUNNING)

solves that.

Getting a bit philosophical (i.e. Plato-mode), all nodes should behave equally, regardless of whether they are async or sync.
These changes we propose (and use) go in that direction.

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