sartography / SpiffWorkflow

A powerful workflow engine implemented in pure Python
GNU Lesser General Public License v3.0
1.69k stars 313 forks source link

Intermediate Boundary Error and Custom Service Task with v2.0.0rc0 #334

Closed asauve2 closed 1 year ago

asauve2 commented 1 year ago

Runtime error and exception handling have been somewhat difficult to get right with v1.2.1.

Problems observed:

Goal:

class CustomServiceTask:
    def _run_hook(self, task):
        raise RuntimeError() # => execute task flow linked to intermediate boundary error

Actually issue #312 seems related but actually never got something hapening with v1.2.1 when an exception was raised. Fix in previous version was to generate a task branch from _BoundaryEventParent which looked excessive. Didn't found either clues in the 2.0.0rc0 documentation for now.

How to get error branch working from script task exception?

In practice the workflow below (edited camunda v7) looks ok

boundary-error-script-trigger

It produces the following tasks graph:

- TASK: <Task object (Root) in state COMPLETED at 0x7ff1d4a22a90>
    - children=[<Task object (Start) in state READY at 0x7ff1d3ba1580>]
  SPEC: <SpiffWorkflow.bpmn.specs.control.SimpleBpmnTask object at 0x7ff1d3c14be0>
- TASK: <Task object (Start) in state READY at 0x7ff1d3ba1580>
    - children=[<Task object (StartEvent) in state FUTURE at 0x7ff1d3baa8b0>]
  SPEC: <SpiffWorkflow.bpmn.specs.control.BpmnStartTask object at 0x7ff1d3c14dc0>
- TASK: <Task object (StartEvent) in state FUTURE at 0x7ff1d3baa8b0>
    - children=[<Task object (dummy_task.BoundaryEventParent) in state FUTURE at 0x7ff1d3baa220>]
  SPEC: <SpiffWorkflow.bpmn.specs.defaults.StartEvent object at 0x7ff1d3ba1640>
- TASK: <Task object (dummy_task.BoundaryEventParent) in state FUTURE at 0x7ff1d3baa220>
    - children=[<Task object (dummy_task) in state FUTURE at 0x7ff1d3baa880>, <Task object (ErrorEvent) in state MAYBE at 0x7ff1d3baa7f0>]
  SPEC: <SpiffWorkflow.bpmn.specs.control._BoundaryEventParent object at 0x7ff1d3ba1c10>
- TASK: <Task object (dummy_task) in state FUTURE at 0x7ff1d3baa880>
    - children=[<Task object (EndEvent) in state FUTURE at 0x7ff1d3baa9a0>]
  SPEC: CustomScriptTask(bpmn_id="dummy_task", script="""raise RuntimeError("ERROR_TRIGGER")""")
- TASK: <Task object (EndEvent) in state FUTURE at 0x7ff1d3baa9a0>
    - children=[<Task object (boundary_event_dummy.EndJoin) in state FUTURE at 0x7ff1d3baaa30>]
  SPEC: <SpiffWorkflow.bpmn.specs.defaults.EndEvent object at 0x7ff1d3baa550>
- TASK: <Task object (boundary_event_dummy.EndJoin) in state FUTURE at 0x7ff1d3baaa30>
    - children=[<Task object (End) in state FUTURE at 0x7ff1d3baaac0>]
  SPEC: <SpiffWorkflow.bpmn.specs.control._EndJoin object at 0x7ff1d3ba1070>
- TASK: <Task object (End) in state FUTURE at 0x7ff1d3baaac0>
    - children=[]
  SPEC: <SpiffWorkflow.bpmn.specs.control.SimpleBpmnTask object at 0x7ff1d4a22ac0>
- TASK: <Task object (ErrorEvent) in state MAYBE at 0x7ff1d3baa7f0>
    - children=[<Task object (error_handler) in state MAYBE at 0x7ff1d3baa850>]
  SPEC: <SpiffWorkflow.bpmn.specs.defaults.BoundaryEvent object at 0x7ff1d3ba1ee0>
- TASK: <Task object (error_handler) in state MAYBE at 0x7ff1d3baa850>
    - children=[]
  SPEC: CustomScriptTask(bpmn_id="error_handler", script="""print("ERROR_HANDLER")""")

When running the workflow engine, the RuntimeError() exception is raised as expected, but not caught and the program stops. This looks consistent with issue #312 but I dont get the intuition on how to run the error handling branch ?

Except from

But that looks like way too much effort for a casual error handling

Problem with custom service task

Apart from the previous case, a CustomServiceTask raised new issues.

Let look at the workflow below (camunda 7 editor)

boundary-error-div-by-zero

The main difference now is that we have

class CustomServiceTask()

Extending SimpleTask calls base.__init__() instead of BpmnMixin.__init__() raises an error when spiff v2.0.0rc0 tries to access the lane parameter for boudary event init.

The resulting task graph is now

- TASK: <Task object (Root) in state COMPLETED at 0x7f1261b83a90>
    - children=[<Task object (Start) in state READY at 0x7f1260d02580>]
  SPEC: <SpiffWorkflow.bpmn.specs.control.SimpleBpmnTask object at 0x7f1260d75be0>
- TASK: <Task object (Start) in state READY at 0x7f1260d02580>
    - children=[<Task object (StartEvent_1) in state FUTURE at 0x7f1260d0a850>]
  SPEC: <SpiffWorkflow.bpmn.specs.control.BpmnStartTask object at 0x7f1260d75dc0>
- TASK: <Task object (StartEvent_1) in state FUTURE at 0x7f1260d0a850>
    - children=[<Task object (Activity_div_by_zero) in state FUTURE at 0x7f1260d0a310>]
  SPEC: <SpiffWorkflow.bpmn.specs.defaults.StartEvent object at 0x7f1260d02640>
- TASK: <Task object (Activity_div_by_zero) in state FUTURE at 0x7f1260d0a310>
    - children=[<Task object (EndEvent) in state FUTURE at 0x7f1260d0a8e0>]
  SPEC: CustomServiceTask(bpmn_id="Activity_div_by_zero", action="""1 / 0""")
- TASK: <Task object (EndEvent) in state FUTURE at 0x7f1260d0a8e0>
    - children=[<Task object (Process_boundary_error_triggered.EndJoin) in state FUTURE at 0x7f1260d0a940>]
  SPEC: <SpiffWorkflow.bpmn.specs.defaults.EndEvent object at 0x7f1260d0a5b0>
- TASK: <Task object (Process_boundary_error_triggered.EndJoin) in state FUTURE at 0x7f1260d0a940>
    - children=[<Task object (End) in state FUTURE at 0x7f1260d0a9d0>]
  SPEC: <SpiffWorkflow.bpmn.specs.control._EndJoin object at 0x7f1260d02070>
- TASK: <Task object (End) in state FUTURE at 0x7f1260d0a9d0>
    - children=[]
  SPEC: <SpiffWorkflow.bpmn.specs.control.SimpleBpmnTask object at 0x7f1261b83ac0>

The problem now is that no _BoundaryEventParent is created.

What should be done to get

essweine commented 1 year ago

You'll need to catch the exception within the task, constuct an event using ErrorEventDefinition and send it to the workflow via the workflow.catch method (the workflow is accessible via the task)

Here is a small example.

from SpiffWorkflow.bpmn.specs.event_definitions import ErrorEventDefinition
from SpiffWorkflow.bpmn.specs.mixins.bpmn_spec_mixin import BpmnSpecMixin
from SpiffWorkflow.bpmn.specs.mixins.service_task import ServiceTask

def do_work(data):
    1 / 0
    return True

class ServiceTask(BpmnSpecMixin, ServiceTask):

    def _run_hook(self, task):
        try:
            return do_work(task.data)
        except ZeroDivisionError as zde:
            # The string can be whatever you want, but needs to match the name of the event in the BPMN diagram
            # An error code can also optionally be provided; if you do this, keep in mind that the XML parser will turn it into a string
            error_event = ErrorEventDefinition('ZeroDivisionError')
            # The workflow can be accessed via the task and the event handlin mechanism can be called here
            task.workflow.catch(error_event)
            return False
        except:
            # This will cause the task to change to the ERROR state but not stop the workflow
            # Alternatively you could raise an exception here, in which case the workflow will error out
            return False

Currently there's no way to attach the actual exception to the error event, but this is something that I'll likely be adding soon.

asauve2 commented 1 year ago

Thanks @essweine, the above example workeed indeed with the ScriptTask above. At least now I can use the correct idiomatic form with error Events.

However an issue remain in the case of custom ServiceTask (second plot in top comment): neither the ErrorEvent task nor the parent _BoundaryErrorParent of the handler Task appears in the task list. The task list remains the same as in the ecustom service task example above.

Any idea why the handler branch is not created?

A second lesser problem is that in the general case, the CustomXXX class does not know the bpmn_id of boundary events attached. I can get them by tree search from the children tasks of _BoundaryEventParent (once it is created ;-) ), but I suspect this will break in future releases. Is there a better way to search Intermediate boundary events attached to a task?

asauve2 commented 1 year ago

As a side note for general case and future extensions:

There is a throw method here:

SpiffWorkflow.bpmn.specs.event_definitions.EventDefinition.throw(my_task)

It seem better practict to generate the throw from the task itself when it is accessible intead of of hard coding the ErrorEventDefinition() from the user code. Is this method expected to produce the same result as the explicit event instanciation in the code above?

essweine commented 1 year ago

As far as throwing from the event definition is concerned: the event definition is basically part of a task spec, and task specs control the behavior of the task, so that's why the method is there rather than on the task.

I thought you'd resolved the initialization problem with the service task. For that, I'd inherit only from SpiffBpmnTask or BpmnSpecMixin and not from Simple.

One thing to understand about SpiffWorkflow is that it is basically two not entirely compatible libraries crammed together as one.

The core (SpiffWorkflow.specs) has some interesting stuff, but it is largely ignored by the BPMN packages (bpmn, spiff, and camunda). One of my long term goals is to make better use of the core task specs, but for the time being, they should probably be ignored by anyone using the BPMN packages. Simple is essentially like a final class (even though that concept doesn't really exist in python, that's probably the best way to think about it); it doesn't extend TaskSpec and just makes it hard to override methods. For BPMN users, BpmnSpecMixin (or SpiffBpmnTask if you use our extensions) is the BPMN equivalent of Simple, so I would recommend inheriting from one of those.

In your case, you might want to use SpiffBpmnTask, as it hews closer to the BPMN spec for MultiInstance tasks than the MI task in the Camunda package. Our Camunda MI task is based on an interpretation of the fields available in an older version of Camunda. I don't know if the fields have changed, or if the interpretation was even right in the first place. (If you're using a current version of Camunda's editor and find our Camunda task is wrong and want to update how the fields are treated, we'd welcome the contribution.)

However, even if you're just inheriting from one of these, there could still be a problem. The way we handle boundary event tasks is a little wonky. To be perfectly honest, I think it's terrible and would love to have an excuse to redo it :). So if the issues go beyond the BPMN lane problem (which should be resolved by dropping inheritance from Simple), let me know and I'll look at it further. The long and short is the boundary event and the task it's attached to are controlled by another already completed task (a _BoundaryEventParent)

I realize I don't see this in your graph output, though I don't know if this is a rseult of some other modification you're making to the task tree or just an artifact of how you're outputting it. There should be a task called execute.BoundaryEventParent which sets execute to READY (meaning it could be run) and set ErrorOnDivision to WAITING (meaning it can catch the event if it's sent to the workflow). So making sure you have a _BoundaryEventParent would be the next thing to check.

asauve2 commented 1 year ago

Finally this is fixed!

The issue about lane attribute missing has been in fact previously fixed using your code snippet above. All boils down to an error in the CustomServiceTask parser returning self.spec.

I put here the correct overloading for future readers:

from SpiffWorkflow.bpmn.parser.TaskParser import TaskParser 
class CustomServiceTaskParser(TaskParser):
    def parse_node(self, *arg, **kw):
        task_spec_or_parent = TaskParser.parse_node(self, *arg, **kw)
        # after parse_node(), self.task is equal to self.task 
        # if there are NO boundary events attached

        # custom attribute processing there

        # here the mistake was to return self.spec
        return task_spec_or_parent

In practice the _BoundaryEventParent was really created but not attached to the task tree when returning mistakenly self.spec. This behaviour has bitten me for the second time. As you commented above, this is a bit wonky and an update on this part would really be welcome.

As we are using spiff with a single ServiceTask class implementation which has no prior knowledge of the bpmn_id of any boundary event I had to find a way to implement try / catch feature. The result is posted in #335 as it is a slightly different topic.