SimpleActionState not reusable after preemption if goal not preempted

[jorgenfb]

I am using SimpleActionState to call an action server which does not behave as SimpleActionState expects. The server sets the goal status to succeeded (it does have some results to share, so one could argue this to be valid) when preempted.

The problem is that SimpleActionState only run self.service_preempt() if the state is preempted AND the GoalStatus equals PREEMPTED (at this line). The next time this state is entered, then its just preempts early due to the preempt flag being set (at this line).

The fix is to make sure that self.service_preempt() is called when preempt is requested, regardless of the goal status.

One could also consider changing the outcome of the state is such a situation. Today it results in 'aborted' . I would argue that it should either be 'preempted' since the state is actually preempted, or 'succeeded' since the goal succeeded.

I am happy to provide a PR if we can decide on the behavior.

[sjhansen3]

Is there an update on this?

[jorgenfb]

No, I was trying to start a discussion to find out how to best handle this case. I haven't seen any interest / activity here until now, and I don't want to do an PR that either would get rejected or just left abandoned.

How would you prefer it to behave? Lets say the action return success when preempted. Do you want it to preempt, er succeed?

(I ran into this issue with an action that collected data until cancelled, then returned the collected data with a success result).

[sjhansen3]

I realize now I was having a different issue than you.

My issue related to preemption for SIGINT - to be honest I'm still a little unclear on the built in behavior for this. It seems the standard smach statemachine does not handle it.

Here is what I was trying to do:

# create a state machine which has the action service in it
sm = SomeStateMachine()
 
# setup safe termination
def handler(signum, frame):
    rospy.loginfo('caught CTRL+C, exiting...')           
    sm.request_preempt()
    signal.signal(signal.SIGINT, handler)

# (1) start the state machine
sm.execute(parent_ud)

# (2) wait a few seconds into the action service execution and press Ctrl +C

My hope was that when signal.SIGINT (ctrl+C) is called the goal is cancelled. It appears that python 2.7's signal does not interrupt blocking threads, while python 3.3 fixes this issue.
https://stackoverflow.com/questions/25676835/signal-handling-in-multi-threaded-python

I rewrote the action service without the threading and signal with preemption worked just fine.

[krixkrix]

I have made a quick workaround myself that does exactly what you suggest:

... to make sure that self.service_preempt() is called when preempt is requested, regardless of the goal status.

I think this is the least bad solution.

[krixkrix]

I believe this issue is very similar and involves the same considerations about the expected behavior for state preemption.

[StephanHasler]

Actually it can be reduced to the simple logic:
After a state has been executed, in each case its preempt_requested flag should be set to false.
Either the state does it by itself, in this way signalling the container that the request was serviced.
Or if the state did not do it, the container should do it afterwards. I think currently the state machine container just does it when transitioning to a state but not for terminal states.

[jorgenfb]

Yes. That is a minimum. It also seems to be quite important to actually have the outcome be preempted. If others parts of the state machine requested a preemption is should do so regardless of what the action returns. Otherwise you might end up in an unintended state. One example we have seen in our systems happens when the state machine is instructed to shut down. If it does not preempt the state machine could continue doing the next state instead of shutting down.

We have been using this solution (preempt is serviced and returned regardless of the actual response from the action) in production for some months. The change is fairly simple and I could provide a PR if it is something the project wants.

[StephanHasler]

I guess your proposed solution will solve the problem for most use cases.
But still we should try to find an even more general solution that is not just optimal for the one or the other use case. Maybe for this a more general discussion of the overall preemption strategy is needed. Here is a comment of mine to a related topic:
#37 (comment)

For your example, in case a state received a preempt_request, you assume the next state should not be executed (e.g. in case of shut down). But this is not a general solution, because it might be mandatory to execute the next states to finish the state-machine in a clean way. The user should be in control of this and not the developer of the SimpleActionState. A good preemption strategy should help to leave a container in the most controlled and not just the quickest way.

Actually that a state returns 'preempted' has no meaning in itself (or should not have).
The two things that matter when leaving a state/container are:

1. Does the state outcome lead to a transition to a state or is it terminal?
2. Should the preempt_request be propagated to the next state?

So a sufficient preemption strategy for a container might be:
Signal every contained state that we like to reach a terminal state. For this the preempt_requested signal can be used. When the container reaches a terminal state, then we can reset the preempt_requested for all contained states. But there is actually no need for a state to explicitly service a preempt or reset the flag, because reaching a terminal result already means it was serviced.

Furthermore, if the action of a SimpleActionState actually succeeded despite a preempt request, but you just return 'preempted', you might throw away information.

I will try to think of all possible cases a user might want to leave a SimpleActionState in the context of a larger state machine, and I will post this here. Maybe this would help to get a more general picture. I would be happy to discuss these points further.

[jorgenfb]

Great insight! This is exactly why I opened this discussion.

[jorgenfb]

I can add that the same issue also exists for ServiceState. If preempted while calling a service the preempt requested flag is left set and on the next run of the state it is preempted immediately.

[StephanHasler]

Yes, you are right. All ROS states have similar flaws regarding preemption.

You could of course check the preempt_requested in the response_callback.
Or you could add more service_preempt()'s at several places inside the ServiceState.execute().

But still the preempt request could arrive directly before the ServiceState.execute returns.
You could use a threading lock to ensure that a preempt request can only arrive at defined times during the execute.

But actually as mentioned before I see no need for a state to service_preempt at all.
If the container sets the preempt requested for a contained state, then also the container should be the only one who recalls the request. The only thing the state should do, is trying to return a terminal outcome.

Last week I completely reworked the whole smach stuff with regard to preemption following the assumptions above. I also fixed most problems/typos/strange thing that PyCharm made me aware of. So it is a rather big change in total. But I think my solution is pretty consistent. Unfortunately, I have not found time to test it yet.

[jorgenfb]

Great! If you would like for me to review any of it just create a PR (a draft one if you are not ready yet) and I'll have a look.

We did actually discover yet another problem for concurrent containers. Where two states returned at the same time. Since the concurrent container was set to preempt other states when the first one terminates the preempt flag was left active on one of the two terminating states (a statemachine), making it preempt immediately on the next run. Hopefully this will also be take care of. I tried to solve it with your approached, recalling any unhandled preempt requests when the concurrent container terminates. It seems to work well. I eager to see what you came up with.

[jack-frampton-dx]

This issue works similarly in concurrent state machines, I made a comment here first about a workaround: #88

This issue works similar to states within a concurrent state machine. Albeit slightly differently, and more alike to the issue here that you posted: #52.

I have not looked too deeply into the source code, but I have found a potential fix / workaround in local code.

Issue: State preempts early if previously preempted.

You cannot resolve this issue by simply resetting the _preempt_requested flag to False (either explicitly, through recall_preempt() or service_preempt()) within the execute() function. It prevents the state from being preempted again.

Resetting the _preempt_requested flag to False can be done "cleanly" (allowing the state to be preempted again) if the preempt flag is reset outside the container requiring the reset.

My workaround was to recall_preempt() on the appropriate states within the custom concurrent state machine class before returning its own outcome (via concurrence_outcome_cb).

I hope this helps someone who also comes across this "early preempt" issue.

Common cmd line output: [smach_ros]: Preempt requested on state machine before executing the next state. (so someone can find this)

Edit: I realise this was also mentioned above!

Edit2: Another workaround is self.preempt_recall() specifically just before you return an outcome in the state itself. Rather than at the start / anywhere else in the execute() function. Weird. Example:

if self._preempt_requested:
                self.recall_preempt()
                return outcomes.PREEMPTED.value

(Latter method works in deployment but during some unit tests it causes the concurrent state machine to raise an unspecified exception, former passes both)