Backtracking issue

[vince]

Hi Paul,

I'm trying to test the backtracking feature of Sarasvati, and I tun
into an issue, butI'm not sure if I do something wrong or if Sarasvati
does :)

So here's the scenario:
* I have 4 nodes
* there's a named arc between the 3rd and 4th nodes
* When I execute the 4th, in the execute() method of that node, I call
engine.complete (token) on the current node token, and immediatly
after that, I call engine.backtrack(), passing it the node token
generated earlier on the 2nd node of my graph.

The problem is that it ends in a NullPointerException in
BaseEngine#executeNode because the NodeToken it gets is, well, null.
This is because the line resultToken = visitor.backtrack(); returns
null (line 480 in BaseEngine)

I noticed several things that seem a little odd to me and that
combined together explain the NPE:
- In BacktrackTokenVisitor#visit(final ArcToken token), the arc tokens
are put in parentMap only if their ExecutionType is Backtracked; in my
scenario it's never the case, they're always 'Forward'
- At some point in BaseEngine, BacktrackTokenVisitor#backtrack() is
called. That methods loops on the contents on the class variable named
queue. Now in my scenario, there's only one NodeToken in that queue,
the 4th one. That's because the visit() method (called from
CreateOrderTokenTraversal) only puts node token without child (arc)
tokens in that queue. Inside that loop, the corresponding node's
backtrack() method is called, then a check is made to see if the
current node token (the one we just removed from the queue) is the
destination token, in other words, the token we passed to
BaseEngine#backtrack(). In my scenario, it's not. That means that
backtrackToken() will be called on the current token.
In that method, in my scenario, the code at line 223 is executed since
my node token is complete, its execution type is not backtracked and
its child tokens list is empty. So, the token is marked as
backtracked. After that there's a for loop that calls getParents(),
which tries to get a node token's parents from parentMap...which is
empty. At this point I don't know if it's a problem or not. On the
other hand, when we come back in backtrack(), resultToekn has not been
changed and is still null, we exit the loop because there was only one
node token in the queue, and so the method returns a null token.

So far I don't really know what is going wrong, I was hoping you could
give some help :-)
I'll have a deeper look tomorrow, but maybe in the meantime you can
pinpoint what's wrong in this (lengthy) explanation...

Cheers
Vincent

[Paul Lorenz]

Because the backtracking code is the most complicated thing in Sarasvati, every time I find a problem, I create a unit test for it, to make sure I don't cause regressions.

So for me to create and fix this, I need the graph you're using the execution pattern.

Is the following correct?

NodeA --- (arc1 unnamed) --> NodeB --- (arc2 unnamed) --> NodeC --- (arc3 someName) --> NodeD

with an execution of

nodetoken1 on NodeA ->
  arctoken1 on arc1 ->
nodetoken2 on NodeB ->
  arctoken2 on arc2 ->
nodetoken3 on NodeC ->
  arctoken3 on arc3 ->
nodeToken4 on NodeD
 --> ? (shouldn't the workflow be complete here, and not allow further interaction?)
 
then backtrack to arctoken2.

The issue may have to do with calling backtrack from inside execute, I'm not sure. I don't think the named arc has any effect, but I can try that as well.

I added a unit test called BacktrackCompletedTest. I modeled it on your description as best I could. It succeeds when I run it. Maybe you can take a look and see how it differs from your code.

cheers,
Paul

[Vincent Kirsch]

Hi Paul,

Your scenario is indeed correct. In the actual graph, NodeD isn't the last node of the graph, but it doesn't have any arc going out of it.

As I said, I'll spend some more time on it tomorrow, maybe I'm doing something wrong too. But the scenario seems pretty simple though :)

Thanks

Vincent

2009/8/31 Paul Lorenz <plo...@gmail.com>

[Paul Lorenz]

It might be engine specific, as the unit test works with the mem engine. Are you using hib engine, or a custom engine? If it's a custom engine, perhaps the token parent/child relationship isn't getting created properly?

cheers,
Paul

[Vincent Kirsch]

Hi Paul,

In the rush (the problem showed at the end of my working day), I overlooked the line 281 in BacktrackTokenVisitor

return (related == null ? token : related).getParentTokens();

So indeed you guessed correctly, my NodeToken didn't have its parent ArcTokens set correctly...

OTOH, I found that during the batcktracking process, the events for created node tokens and arc tokens aren't sent by BaseEngine. Is this on purpose? If not (and I hope it's not because I rely on those events :) I'll fix and commit it.

Thanks for your time!

[Paul Lorenz]

I'll add those missing events. I think I'll also add additional events for backtracking.

cheers,
Paul

[Vincent Kirsch]

As you wish.

While we're talking events, I was wondering if it would make sense to have events for all the possible steps of an execution cycle; i.e. send an event when a token is marked as backtracked, or marked as complete, etc. ? What do you think?

To me it would be really helpful because since I use a custom engine and a different DB schema, I rely on the engine's event to create and update records in my DB... So far, there aren't such events, so I can't really update completion dates etc. in the DB. Or maybe there's another way?

Thanks

Vincent

2009/9/1 Paul Lorenz <plo...@gmail.com>

[Paul Lorenz]

I just added backtracked events. We already had completed events (though they were missing from backtracking, I've added them now).

The only place we don't have completed events now is for when a token is skipped or discarded. I could add them, but then we are firing two events for one action. I'm on the fence here. Options?

Paul

[Vincent Kirsch]

Hello,

In BacktrackTokenVisitor, you added things like 

ArcTokenEvent.newBacktrackedEvent( engine, token );

Shouldn't they be 

engine.fireEvent(ArcTokenEvent.newBacktrackedEvent( engine, token ));

? Just creating the event won't do much :-)

If yes, I already made the changes locally, I can commit those if you wish.

Correct me if I'm wrong, but we already have skipped and discarded events, they're sent by BaseEngine#executeNode(), no?

On the other hand, I'd be happy to add an ARC_TOKEN_PROCESSED event, so that I can easily update the pending field in the arc tokens table. I also already did that locally, and if you're okay with it, I can commit it too.

As for your concern of sending several events for one action, to me it's no big deal because I'm not sure many people will actually use this events mechanism. But OTOH, those - like me - who do will probably need to get as many events as they can in order to, say, reflect the execution changes in a DB. Plus, it's not like it kills Sarasvati's performance ;-) So I'd say it's up to you eventually, but my opinion is that it's neither useless nor overkill.

Cheers!

[Paul Lorenz]

Doh! Thanks for catching that. I'm updating that API, which lead me to some other changes. I'll check them in a bit.

cheers,
Paul

[Paul Lorenz]

Ok, checked in.

The pattern of

engine.fireEvent( NodeTokenEvent.newCompletedEvent( engine, token ) );

has been changed to

NodeTokenEvent.fireCompletedEvent( engine, token );

which removed a bit of duplicated code. I also added the ARC_TOKEN_PROCESSED event type. I removed the engine arguments from markProcessed, markCompleted and recordGuardAction (and renamed recordGuardAction to setGuardAction). I had the engine there so that subclasses could handle processing. However, if we have a complete set of actions, the Engine can just register a listener to catch those events.

On that theme, I added a TokenSetCompletionListener which handled removing completed arc/node tokens from their token sets.

I also added support for application contexts. I thought maybe, someday, you might have multiple applications using Sarasvati in the same JVM. In that case you wouldn't want global per JVM listeners, you'd want global per application listeners. So now when you create an engine you can specify the application context. Engines are allowed to set which listeners are added to a new application context by default. Right now the only listener which is always added is the one for token sets.

I still need to add some more unit tests to exercise the event framework.

Do these changes meet your needs, Vince?

cheers,
Paul

[Vincent Kirsch]

Hi,

Well it looks good. I'll test all that stuff but since I already had those changes locally before your check-in, it should go smoothly.

I have 2 questions though:

- Shouldn't we fire a skipped event in BacktrackTokenVisitor#backtrackToken (line 252)? Again, it's ready for check-in locally here if you wish

- I think it might be useful to add an additional check in BaseEngine#executeQueuedArcTokens():

      while ( !process.isArcTokenQueueEmpty() )

 should be

      while ( process.isExecuting() && !process.isArcTokenQueueEmpty() )

In one of my tests, a node execution triggers a call to Engine#cancelExecution, but there were other node/arc tokens still active. What happened is that at some point, executeQueuedArcToken is called, and since it doesn't check for process completion, and since completion just sets the execution state without removing active tokens, the execution continued even though it was just cancelled. Now, this race condition might happen because of the way I tested my execution (I'm checking this at the moment but it will take some time), but still, I think it's better to be double safe there, what do you think? I can commit it too if you wish.

Thanks for all

Vincent

2009/9/2 Paul Lorenz <plo...@gmail.com>

[Paul Lorenz]

Sounds reasonable. Go for it.

cheers,
Paul

[Vincent Kirsch]

Hello,

Would it also seem reasonable to add events PROCESS_CREATED, PROCESS_CANCELLING and PROCESS_COMPLETING ?

That would be really helpful to me!

Thanks,

Vincent

2009/9/3 Paul Lorenz <plo...@gmail.com>

[Paul Lorenz]

I added PROCESS_CREATED, PROCESS_PENDING_COMPLETE and PROCESS_PENDING_CANCEL, to match the process states. This changes some semantics, since we used to call PROCESS_COMPLETED when the process state changed from execution to pending complete, rather than when it went from pending complete to complete. Now the event more closely correlates to the state.

The EventActions had to be updated as well.

I went ahead and shifted the event bit masks, since they only became relevant yesterday. We can keep them in flux until RC3 is released, at which point they should remain constant.

Does the change look good? Are there any other events we're missing?

cheers,
Paul

[Vincent Kirsch]

Paul,

Many thanks, it looks great!

I think we covered pretty much all the possible events...

Cheers,

Vincent

2009/9/4 Paul Lorenz <plo...@gmail.com>