Problem with handling a client which was used in a hierarchical BehavioralFsm in a new instance of the same BehavioralFsm

[damienwhaley]

We are using a hierarchical BehavioralFsm and we have run into a problem when we try and handle a client in a new instance of the hierarchical BehavioralFsm, it was not reporting the correct compositeState() and it was not correctly delegating control of the state to a child machine when you called a handler. I've tried to illustrate the problem in psuedo code:

var childMachine = new machina.BehavioralFsm({
    namespace: 'child',
    initialState: uninitialized,
    uninitialized: {
        _onEnter: function(client) {
            this.transition(client, 'one')
            this.handle(client, 'doIt');
        }
    },
    states: {
        one: {
            doIt: function (client) {
                this.emit('SOME_WAITING_REQUIRED', client);
            },

            finishedWaiting: function(client) {
                this.transition(client, 'two');
            }
        },

        two: {
            _onEnter: function(client) {
                this.handle(client, 'jumpAhead');
            }
        }
    }
});

function parentMachineFactory() {
    var machine = new machina.BehavioralFsm({
        namespace: 'parent',
        initialState: uninitialized,
        states: {

            uninitialized: {
                _onEnter: function(client) {
                    this.transition(client, 'next')
                    this.handle(client, 'doIt');
                }
            },

            next: {
                _child: childMachine,

                jumpAhead: function (client) {
                    this.transition(client, 'done');
                }
            },

            done: {
                _onEnter: function(client) {
                    this.emit('COULD_NOT_BE_MORE_DONE', client);
                }
            }
        },

        doIt: function(client) {
            this.handle(client, 'doIt');
        },

        finishedWaiting: function(client) {
            this.handle(client, 'finishedWaiting');
        }
    });

    return machine;
}

var client = {};

parentMachine1 = parentMachineFactory();
parentMachine2 = parentMachineFactory();

parentMachine1.doIt(client);
parentMachine1.compositeState(); // = next.one

parentMachine2.doIt(client);
parentMachine2.compositeState(); // next <-- should be next.one
parentMachine2.finishedWaiting(); // get a nohandler event <-- should have delegated control to client

This would mean that the the parentMachine2 would not progress, and it would be stuck in "next" state and would not move on. This is what we ran in to.

[ifandelse]

@damienwhaley Thanks so much for the detail, and for the PR that included tests! I'll try and look this over and get back if I have any other questions before merging.

[ifandelse]

@damienwhaley - I've started to look into this. One thing I noticed is that the parent FSM has the same namespace of parent for both parentMachine1 and parentMachine2. This could prove to be a problem since machina stamps the client objects with a __machina__ prop that contains the namespaces of the FSMs that are acting on it, and these two FSMs will target the same namespace. While it does work without the bug you ran into when you use different namespaces, what you've identified definitely seems to be a problem...I'll keep you posted on what I find.

[ifandelse]

@damienwhaley - OK...thanks to your example above, and the PR you submitted, I definitely have a good picture of what's going on. One thing I'd love to do, if possible, is find a way to make the getChildFsmInstance behavior consistent between what happens inside handle and inside transition, so I once I've gotten some sleep I plan to look at taking the additions you made inside handle and moving them behind a common call (like getChildFsmInstance) that handle and transition share. Really appreciate you taking time on this!

[damienwhaley]

@ifandelse Exactly, the problem we wan in to was when using a new instance of the same machine. A common function for both transition and handle would be a good idea.

One thing which may make things easier is that when the child machine is added to the parent, it is wrapped in an "instance" node. Perhaps if you don't wrap the child in the instance node, then you may not need to change much code. The common code would just add the hierarchy information. Just an idea!

The reason we found the bug was that we were creating a new machine in a callback closure. Not your typical way of using machines, I'm sure.

Looking forward to seeing what you come up with.

[ifandelse]

@damienwhaley Man I'm so sorry it's taken me 3 months. Once I got into Nov and Dec, my free time evaporated (working on changing that!) - but I took this week off and wanted to catch up on things like this. I have a branch that's a release candidate for 2.x (opted for a major version bump since I'm changing the build and UMD wrapper in addition to fixing this issue). No pressure, but if you end up with time to pull this branch down to see if it addresses the issue you were experiencing, let me know how it goes.

[damienwhaley]

Hi @ifandelse - thanks for the update. I've had a quick look and this version appears to be working for us. I need to do some more testing, but it looks good. I'll respond back on Monday with my findings.

[damienwhaley]

@ifandelse I've had the opportunity to test the release candidate branch with our internal unit tests, and the code works perfectly. I'm happy to say that you've resolved the issue we have found.

One thing I found, and it may be because of the way I included the /lib/machina.js in our project is that I had to include the following line at the top of the file:

_ = require('lodash');

I'm not sure if that is just a packing or script output thing when producing files for the lib directory. I though I would mention it anyway.

Thanks!

[ifandelse]

@damienwhaley - I'd be interested to know how you included machina. The UMD that webpack produces will require lodash for you in CommonJS scenarios. If you're using AMD, it looks for a module registered to the key "lodash". It should only require that _ is at the root object if you're in a browser scenario and not using CommonJS or AMD.

[damienwhaley]

@ifandelse I included machina in a really simple way. I had my project cloned, and machina cloned and I just included a symlink to the lib/machina.js file. The package.json just includes that one file, so it should have worked, but did not for me. To fix it I just added the line var _ = require('lodash) to the top of the file above the UMD.

I'm using node 4.2.x.

I'll try including the cloned machina properly (symlink in the node_modules directory) and report back on my findings.

[damienwhaley]

@ifandelse Sorry - I've been super busy. Short answer is that the packaging did not seem to work, but I will put together a sample app which which hopefully demonstrate whether it works or not!

[damienwhaley]

Hi @ifandelse I've finally had the time to put together an example which shows that the packaging is not quite correct for Node.js.

Please clone https://github.com/damienwhaley/machina-example and then npm install, and run node index.js.

If you add the line var _ = require('lodash'); at the top of the machina.js in the node-modules it will allow the example to run.

However you've packaged the module is not quite right.

$ node --version
v5.7.1

I hope this helps!

[ifandelse]

Thanks @damienwhaley I'm pulling this down today and will check it out.

[ifandelse]

@damienwhaley Well I feel like a dummy :smile:. I found the issue. The emitter.js file is missing a lodash require. I'll get the release candidate branch updated. I tested it locally with the sample repo you provided and it works now. Thank you very much for taking time to do that!

[ifandelse]

@damienwhaley FYI - pre-release v2.0.0-1 is up on npm now.