pvm and end of executions

[porcherg]

In the current pvm, the end of execution is not tested:
we can signal a already finished execution, it signals the last node of the process (and redoes the actions related to this node).

A simple test to show that: in the test class org.jbpm.pvm.WaitStateTest, in the test threeNodesInARow, just duplicate the last 2 lines:

execution.signal();
 assertEquals(processDefinition.getNode("c"), execution.getNode());

The test still succeed. The node "c" can be signaled more than one time. No exception is thrown.

I think it should not be allowed to perform actions on a ended execution. Maybe we need to add some checks before each operation on an execution (or we can just set the current node to null in the end() method so that a call to signal throws an exception)

regards,
Guillaume

[camunda]

+1

I experienced this to be a possible error source in jbpm 3 already, so I absolutely agree with you!

Cheers
Bernd

[tom.baeyens]

we could do that, but i have some reservations:

if a client calls signal on an ended execution, there must be a bug in the client code. cause the process doesn't expect an external trigger any more.

so if you start protecting the users from his own mistakes, maybe we need to do it in a lot of other places as well ? guard conditions on transitions are already covered. but are there other ?

you can add some checks in some methods. but how far do you want to go ? do you want to include these checks in getters and setters ? including related runtime data objects like variables, bpel correlation sets, xpdl swimlane instances...

which methods would you like to guard with an already-ended-check like that ?

[camunda]

if a client calls signal on an ended execution, there must be a bug in the client code

I agree, but in circumstances, where I really trigger the execution, it should be checked. Not in getters and setters, only in signalling stuff (or ending TaskInstances, but I am not sure, how this is handled in PVM).

This matches with Design by contract I think, because the contract is to signal correctly, if it was not yet signaled. Otherwiese the client violates the contract, which should be indicated by an exception.

asserts would be a good choise here I think, that's what they made for. And that can even makes the code more readable...

[porcherg]

If we just add to the 'end()' method:

node = null;
transition = null;

the exception

throw new JbpmException("execution is not in a node or in a transition");

when a ended execution is signaled.

If there is a bug in the client code, an exception is raised. There is no need to perform additional checks in other methods.

Is that solution ok ?

Guillaume

[tom.baeyens]

adding node = null; is not ok. there can be different end states in a process and the actual end state can matter.

if a check like that is introduced, it must be with a check on an isEnded property.

make sure that it can be bypassed by calling execution.setEnded(false); or something like that.