Bug in the generated JavaScript code for transition messages within transition actions

[jakhog]

There seems to be a bug in the State.js library, the EventEmitter class, or the way we use them.

Currently with the browser-compiler (xtext branch), the following problem arises: If a message that triggers a transition is sent - while inside the action of another transition - the transition caused by the message occurs, but is later overridden by the original transition that was happening.

I suspect that this is because there is no real FIFO, and messages are actually handled as function calls by the EventEmitter. Perhaps we should implement our own class that uses a proper FIFO that queues the messages so that current transitions are allowed to complete before the next message is handled.

I will have to write a simpler piece of code to re-produce the bug later...

[ffleurey]

I am having some strange issues which could be related but I am the C compiler so it may not be specific to JavaScript... I will post details when I understand what is going on...

[brice-morin]

@jakhog write a test and not just an example :-)

[brice-morin]

Not sure yet, but maybe some well placed setImmediate or some magic functions alike can help proper scheduling events in this case

[brice-morin]

Before, we used to wrap the send action into a setImmediate, which if I remember well, allowed the current transition or state action to terminate before the sent message was handled. The problem was that it sometimes yielded some scheduling that were a bit un-natural (though valid). When instance a sent a message to instance b, the message was not immediately handled by instance b. instance a had to finish its business first. I guess we should wrap send actions into setImmediate when we send a message through an internal port, but not when sending through a normal port. It might still be some problem with "self-connected" non-internal ports, but then we can have a rule in the checker suggesting to use an internal port.

[brice-morin]

Also the setImmediate stuff was before we switched to EventEmitter. But I would first try putting the setImmediate for SendAction on internal ports, as described above, to see if it helps.

[jakhog]

@brice-morin: I will make sure I write a test that fails before I fix it :smile:

If you are talking about what we did with setImmediate, then yes, that is exactly what we tried to accomplish, but it was only to wait for the statemachine to initialize. And after that, the setImmediate is no longer used.

Is there any reason why we shouldn't use setImmediate for all messages? IMO, this would always be beneficial, and I don't really agree with your opinion of un-natural scheduling. As far as I understand, we don't really provide any guarantees about the scheduling of concurrent things in ThingML, only the ordering of messages?

By using setImmediate, we would be very close to the behavior of the C/POSIX generated code which uses the manual FIFO - the way it is implemented now, is more like having the @sync_send annotation on all the messages. By changing it to use setImmediate - we would be using EventEmitter for routing messages, and the JavaScript event loop as a FIFO. Quite clean and nice if you ask me :+1:

The solution of using setImmediate only on internal ports sounds like it wouldn't quite work. What if instance a sends a message to instance b that again triggers a message to instance a. Then we would get right into the same issue again wouldn't we?

[jakhog]

BTW: If we added the setimmediate to all messages, we wouldn't have to have a special case while the statemachine was initializing. It would always be allowed to finish that before any messages would trigger anything.

[brice-morin]

No, I was not talking about this particular setImmediate we used for init. It was before that, when all sent message were wrapped into a setImmediate. Well, I agree that we can generalize the approach, as it was some months/years before (do not remember when I removed it). But it was some complaints by some users (probably more used to synchronous and sequential behavior) that the scheduling was a bit not natural (though, as I said, valid) when everything is running locally, especially when a sequence of messages was sent one after another.

So, let's go for generalizing the setImmediate, which as you point out simplifies/streamlines a few things, as long as the semantics is valid. Then it is up to the users to be trained to async behavior, etc.

[brice-morin]

@jakhog you fix it?

[jakhog]

Ok, I wasn't aware that it was implemented like that before...

I would at least argue that the most important point is to be able to guarantee correct behavior. And I think this FIFO way is by far the easiest way to do that. If someone comes back with a better/different solution later, we can easily change it back.

And we could also implement the @sync_send annotation in JS as well, so that people that need the current kind of behavior have the choice to do so.

[jakhog]

I will fix it!

[jakhog]

This should be fixed by 1b7055b66504f8e11432291c138f9156eddfd399, right @brice-morin?

But I guess we can leave the issue open until I write a proper test to detect it...

[brice-morin]

Yes, should be fixed, but we need a test :-)

[brice-morin]

I have made a test, which seems to pass for all compilers. @ffleurey the problem you briefly mentioned is then another problem and you can post a separate issue (or the test is not good enough... in this case re-open the issue)