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

FSM uses transition which is not registered for event #9335

Open
mbgonicus opened this Issue May 18, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@mbgonicus
Contributor

mbgonicus commented May 18, 2017

Playground: http://tinyurl.com/nxgm3fg

In the playground code we have two states (stateA and stateB) and two transitions (stateA->stateA and stateA->stateB). stateA has events registered like this:

events : {
  "myEvent" : qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE,
  "unusedEvent" : "stateA->stateA"
}

stateA->stateB's predicate always returns false, thus it is never used. The event "unusedEvent" is never fired.

But when firing the fsm event "myEvent", the transition stateA->stateA is invoked (see browser/playground console) - though it is not registered for this event!

So it is possible to invoke transitions by events they have never been registered for - which I would consider a bug.

I think the problem is here: https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/util/fsm/FiniteStateMachine.js#L1093

// We handle the event.  Try each transition in turn until we find one
// that is acceptable.
for (var t in transitions)
{
  var trans = transitions[t];

  // Does the predicate allow use of this transition?
  switch(trans.getPredicate()(this, event))
  {
    case true:
      // Transition is allowed.  Proceed.
      break;

Since the found transition for "myEvent" is "PREDICATE", the fsm searches the transitions to find one whose predicate returns true. But it tries the predicate on all transitions - even if that is not registered for the fired event. For some reason, a transition without a user-defined predicate seems to have a predicate which always returns true.

I presume a fix would have the searched transitions filtered, so only those registered for the event are checked.

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus May 18, 2017

Contributor

My bad - of course you cannot register multiple functions for an event. But I reckon the fsm should only check transitions for which the user has defined a predicate.

Contributor

mbgonicus commented May 18, 2017

My bad - of course you cannot register multiple functions for an event. But I reckon the fsm should only check transitions for which the user has defined a predicate.

mbgonicus added a commit to mbgonicus/qooxdoo that referenced this issue May 18, 2017

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus May 18, 2017

Contributor

As I learn from https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/util/fsm/Transition.js#L72

It is possible to create a default predicate -- one that will cause a transition to be acceptable always -- by either not providing a predicate property [...]

This behaviour seems to be intentional. However, I find it quite odd...

@derrell You are the original author of the fsm? Could you please clarify?

Contributor

mbgonicus commented May 18, 2017

As I learn from https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/util/fsm/Transition.js#L72

It is possible to create a default predicate -- one that will cause a transition to be acceptable always -- by either not providing a predicate property [...]

This behaviour seems to be intentional. However, I find it quite odd...

@derrell You are the original author of the fsm? Could you please clarify?

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus May 18, 2017

Contributor

Code that can be used as a unit test:

var stateA = new qx.util.fsm.State(
  "stateA",
  {
    onentry : function(fsm)
    {
      if (fsm.getObject("wasAlreadyHere"))
      {
        var o = fsm.getObject("testObject");
        o.fail("This state shall not be invoked by the transition");
      }
      fsm.addObject("wasAlreadyHere", true);
    },
    events :
    {
      "myEvent" : qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE,
      "unusedEvent" : "stateA->stateA"
    }
  }
);

trans = new qx.util.fsm.Transition(
  "stateA->stateA",
  {
    nextState : "stateA"
  }
);
stateA.addTransition(trans);

var fsm = new qx.util.fsm.FiniteStateMachine("TestFSM");
fsm.addObject("testObject", this);
fsm.addState(stateA);
fsm.start();
fsm.fireImmediateEvent("myEvent");
Contributor

mbgonicus commented May 18, 2017

Code that can be used as a unit test:

var stateA = new qx.util.fsm.State(
  "stateA",
  {
    onentry : function(fsm)
    {
      if (fsm.getObject("wasAlreadyHere"))
      {
        var o = fsm.getObject("testObject");
        o.fail("This state shall not be invoked by the transition");
      }
      fsm.addObject("wasAlreadyHere", true);
    },
    events :
    {
      "myEvent" : qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE,
      "unusedEvent" : "stateA->stateA"
    }
  }
);

trans = new qx.util.fsm.Transition(
  "stateA->stateA",
  {
    nextState : "stateA"
  }
);
stateA.addTransition(trans);

var fsm = new qx.util.fsm.FiniteStateMachine("TestFSM");
fsm.addObject("testObject", this);
fsm.addState(stateA);
fsm.start();
fsm.fireImmediateEvent("myEvent");
@derrell

This comment has been minimized.

Show comment
Hide comment
@derrell

derrell May 23, 2017

Member

@mbgonicus Yes, that is intentional behavior. The idea is that you can have a list of possible transitions that have predicates, and if none of those predicates pass, it runs the default transition. In my experience, that's the more typical use than not doing anything if none of the predicates for transitions passes. The way around that, of course, if you want to do nothing, is to provide a predicate that returns false.

Does that answer your question?

Derrell

Member

derrell commented May 23, 2017

@mbgonicus Yes, that is intentional behavior. The idea is that you can have a list of possible transitions that have predicates, and if none of those predicates pass, it runs the default transition. In my experience, that's the more typical use than not doing anything if none of the predicates for transitions passes. The way around that, of course, if you want to do nothing, is to provide a predicate that returns false.

Does that answer your question?

Derrell

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus May 24, 2017

Contributor

@derrell Well, I get it's intentional. But I think it's counter-intuitive and violates the principle of least surprise. As in the unit test example:

events :
{
  "myEvent" : qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE,
  "unusedEvent" : "stateA->stateA"
}

// ...

trans = new qx.util.fsm.Transition(
  "stateA->stateA",
  {
    nextState : "stateA"
  }
);

I'd expect that "stateA->stateA" is not invoked when myEvent is fired, because I never defined a predicate for it that would return true for any situation.

Contributor

mbgonicus commented May 24, 2017

@derrell Well, I get it's intentional. But I think it's counter-intuitive and violates the principle of least surprise. As in the unit test example:

events :
{
  "myEvent" : qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE,
  "unusedEvent" : "stateA->stateA"
}

// ...

trans = new qx.util.fsm.Transition(
  "stateA->stateA",
  {
    nextState : "stateA"
  }
);

I'd expect that "stateA->stateA" is not invoked when myEvent is fired, because I never defined a predicate for it that would return true for any situation.

@level420

This comment has been minimized.

Show comment
Hide comment
@level420

level420 Jun 13, 2017

Member

@mbgonicus I'm not sure about the state of this issue. Could you please report back how we could solve this? Best would be a PR. Thank you.
@derrell could you please have a look into this issue? Thank you.

Member

level420 commented Jun 13, 2017

@mbgonicus I'm not sure about the state of this issue. Could you please report back how we could solve this? Best would be a PR. Thank you.
@derrell could you please have a look into this issue? Thank you.

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus Jun 13, 2017

Contributor

Well, I got it that this behavior is intended. But I find it very surprising and misguiding, as a transition I never defined a predicate for actually has a predicate that returns true. So it might be used whenever qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE is used to define the next state for a given event.

Unfortunately, any fix would break current behavior. How is the general strategy for backward-compatibility?

Contributor

mbgonicus commented Jun 13, 2017

Well, I got it that this behavior is intended. But I find it very surprising and misguiding, as a transition I never defined a predicate for actually has a predicate that returns true. So it might be used whenever qx.util.fsm.FiniteStateMachine.EventHandling.PREDICATE is used to define the next state for a given event.

Unfortunately, any fix would break current behavior. How is the general strategy for backward-compatibility?

@derrell

This comment has been minimized.

Show comment
Hide comment
@derrell

derrell Jun 13, 2017

Member

@mbgonicus I'm very reluctant to make a breaking change here. This code has been in use for 10 years. I would, however, be willing to entertain a PR that adds a boolean static variable like ALLOW_DEFAULT_PREDICATE (feel free to propose a better name) that defaults to enabling the current behavior, but if its value is toggled, would enable the new behavior that you feel is preferable.

Member

derrell commented Jun 13, 2017

@mbgonicus I'm very reluctant to make a breaking change here. This code has been in use for 10 years. I would, however, be willing to entertain a PR that adds a boolean static variable like ALLOW_DEFAULT_PREDICATE (feel free to propose a better name) that defaults to enabling the current behavior, but if its value is toggled, would enable the new behavior that you feel is preferable.

@oetiker

This comment has been minimized.

Show comment
Hide comment
@oetiker

oetiker Jun 13, 2017

Member

coultn't this be a 'property' of the statemachine object ?

Member

oetiker commented Jun 13, 2017

coultn't this be a 'property' of the statemachine object ?

@mbgonicus

This comment has been minimized.

Show comment
Hide comment
@mbgonicus

mbgonicus Jun 13, 2017

Contributor

Sounds like a reasonable compromise to me. Whether we do this or not I reckon the behavior should be clearly documented.

Contributor

mbgonicus commented Jun 13, 2017

Sounds like a reasonable compromise to me. Whether we do this or not I reckon the behavior should be clearly documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment