steelbreeze / state.js

DEPRECATED, please see @steelbreeze/state
https://github.com/steelbreeze/state
MIT License
324 stars 39 forks source link

Possible regression on "multiple outbound transitions evaluated true" #32

Closed brice-morin closed 7 years ago

brice-morin commented 7 years ago

I suspect that it could have been some regressions in the way multiple transitions are handled, in particular in presence of composite states. In the ThingML tests (which compiles to state.js), we see a few tests failing because multiple outbound transitions evaluated true, whereas only one of those multiple transitions should in principle evaluate to true at a given time (they are guarded). Has it been any changes there between versions 5.6.10 and 5.11.0 that could have caused a regression?

We also went through quite a big refactoring in ThingML the past few months, so the problem might be on ThingML side. I have to look into more details at the generated JS code, though in principle the generated code should be the same as before. It might as well be a regression in state.js. I will come back to you (either close the issue if it appears it was a problem with ThingML, or try to provide you with a simple example that fail if the problem is with state.js).

mesmo commented 7 years ago

There are two forms of that exception:

Multiple outbound transition guards returned true at ${pseudoState} for ${message}

and

multiple outbound transitions evaluated true for message ${message}

Are you able to confirm which it is? The first one is when evaluating guards after a junction pseudo state and the second is the more general form for states.

Prior to this exception being thrown, the code simply evaluates all outbound transitions guards with this statement:

transitions = vertex.outgoing.filter(transition => transition.guard(message, instance));

The logic then checks for the filtered collection having 0, 1 or many entries and acts accordingly.

If you have an example of a machine that isolates the issue I can look in more detail.

brice-morin commented 7 years ago

It is definitely the more general form multiple outbound transitions evaluated true for message ${message}. I will try to find the most simple ThingML test that highlight the issue. Again, thanks for the rapid feedback 👍

brice-morin commented 7 years ago

OK, here is the state machine:

state-machine

We send the following sequence of message: n n i (look for those values in the guards)

Basically, we start in State I, then

See the whole execution trace:

$ node main.js
[Expected]  012320 [Test] 0123
.....\TestCompositeStates_2_Cfg\node_modules\state.js\lib\node\state.js:1124 throw message;
TestCompositeStates.default.C1: multiple outbound transitions evaluated true for message [object Object]

See below for the whole source code, which to me, looks OK and aligned with the UML diagrams (though we should not exclude a mistake there neither...)

TestCompositeStates_2_Cfg.zip

brice-morin commented 7 years ago

Do you confirm the bug?

mesmo commented 7 years ago

I am just in the process of stripping down your example to leave just the FSM code... then I will confirm.

In parallel are you able to build against the earlier version of state.js?

brice-morin commented 7 years ago

I tried several versions down to 5.3.0 and they behave the same, and raise the exception. I could not directly go to previous version, as it seems it has been some changes in the API between 5.2.x and 5.3.x. We had the ThingML tests off for quite a while, so I do not have any idea when the issue appeared. Also, it might still be some issues in our code (though it pretty much look like it should do what it is supposed to do...) that is not generated the very same way than before. I will try to investigate more.

mesmo commented 7 years ago

I think there may be an issue with you code generation I'm afraid as the transition from C1 to I seems to be defined twice; this is an excerpt from TestCompositeStates.js (lines 67-72)

 TestCompositeStates_TestCompositeStates_C1.to(TestCompositeStates_TestCompositeStates_I).when((testIn) => {
    return testIn._port === 'harnessIn' && testIn._msg === 'testIn' && testIn.c === 'i';
  });
 TestCompositeStates_TestCompositeStates_C1.to(TestCompositeStates_TestCompositeStates_I).when((testIn) => {
    return testIn._port === 'harnessIn' && testIn._msg === 'testIn' && testIn.c === 'i';
  });

There are a few other transitions also defined twice scanning further down the code.

brice-morin commented 7 years ago

Ok, my bad. I should have looked more carefully. Anyway, that is a good news. Should not be too difficult to fix on our side. Sorry for that fake issue!

-----Message d'origine----- De : "David Mesquita-Morris" notifications@github.com Envoyé : ‎23/‎05/‎2017 20:04 À : "steelbreeze/state.js" state.js@noreply.github.com Cc : "Brice Morin" brice.morin1@gmail.com; "Author" author@noreply.github.com Objet : Re: [steelbreeze/state.js] Possible regression on "multiple outboundtransitions evaluated true" (#32)

I think there may be an issue with you code generation I'm afraid as the transition from C1 to I seems to be defined twice; this is an excerpt from TestCompositeStates.js (lines 67-72) TestCompositeStates_TestCompositeStates_C1.to(TestCompositeStates_TestCompositeStates_I).when((testIn) => { return testIn._port === 'harnessIn' && testIn._msg === 'testIn' && testIn.c === 'i'; }); TestCompositeStates_TestCompositeStates_C1.to(TestCompositeStates_TestCompositeStates_I).when((testIn) => { return testIn._port === 'harnessIn' && testIn._msg === 'testIn' && testIn.c === 'i'; });There are a few other transitions also defined twice scanning further down the code. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.