Uploaded image for project: 'camunda BPM'
  1. camunda BPM
  2. CAM-986

Cdi Variable Scope does not work for first instance of Parallel Multi Instance

    Details

    • Type: Bug Report
    • Status: Closed
    • Priority: L3 - Default
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.3.0, 7.3.0-alpha1
    • Component/s: engine
    • Labels:
      None

      Description

      see: org.camunda.bpm.engine.cdi.test.impl.context.MultiInstanceTest.testParallelMultiInstanceServiceTasks()

      Problem Description:

      • there is a three-level execution hierarchy: Process Instance <--- Scope Execution <--- Concurrent Execution
      • the element variable is set on the concurrent executions, but BusinessProcess#getVariable accesses the scope execution for the first MI instance. Thus, the variable cannot be resolved
      • BusinessProcess#getVariable relies on the current execution context, i.e. Context.getExecutionContext()
      • the execution context is updated whenever atomic operations are executed
      • the first MI instance is not part of an atomic operation (cf ParallelMultiInstanceActivityBehavior and MultiInstanceActivityBehavior#executeOriginalBehavior ll 197-201), thus the first concurrent execution is not pushed onto the execution stack

      Solution Possibilities:
      1. Quick: push the first concurrent execution onto the execution context from within MultiInstanceActivityBehavior#executeOriginalBehavior. Ensure that the context is removed again under all circumstances.
      2. Implement MI as a proper concept in the PVM.

        Issue Links

          Activity

          Hide
          thorben.lindhauer Thorben Lindhauer added a comment - - edited

          Hi Ronny,

          Both versions of the MI construct are typically executed sequentially within one transaction if the MI task is entirely synchronous. However, there is a conceptual difference in that Sequential MI is always sequential in a BPMN sense, while Parallel MI is concurrent in a BPMN sense and may be executed in parallel.

          As an example: Assume you have a MI subprocess in which there is a user task (i.e. the task is asynchronous). With sequential MI, there will always be at most one instance of the user task. The next MI instance can only be started when the previous finishes. With parallel MI, all parallel instances of the activity are instantiated immediately. In this case, there will be n active user task instances. In the latter case, you can get true parallelism, since the user tasks may be completed in parallel on different nodes. The following tasks will then be executed truely parallel.

          This difference is also reflected in the code. In SequentialMultiInstanceBehavior, the next instance is triggered in the #leave method. In ParallelMultiInstanceBehavior, there is a for loop in #createInstances (that is called from MultiInstanceActivityBehavior#execute) that immediately executes all instances (until they reach wait states).

          Cheers,
          Thorben

          Show
          thorben.lindhauer Thorben Lindhauer added a comment - - edited Hi Ronny, Both versions of the MI construct are typically executed sequentially within one transaction if the MI task is entirely synchronous. However, there is a conceptual difference in that Sequential MI is always sequential in a BPMN sense, while Parallel MI is concurrent in a BPMN sense and may be executed in parallel. As an example: Assume you have a MI subprocess in which there is a user task (i.e. the task is asynchronous). With sequential MI, there will always be at most one instance of the user task. The next MI instance can only be started when the previous finishes. With parallel MI, all parallel instances of the activity are instantiated immediately. In this case, there will be n active user task instances. In the latter case, you can get true parallelism, since the user tasks may be completed in parallel on different nodes. The following tasks will then be executed truely parallel. This difference is also reflected in the code. In SequentialMultiInstanceBehavior, the next instance is triggered in the #leave method. In ParallelMultiInstanceBehavior, there is a for loop in #createInstances (that is called from MultiInstanceActivityBehavior#execute) that immediately executes all instances (until they reach wait states). Cheers, Thorben
          Hide
          rbraeunlich Ronny Bräunlich added a comment -

          Hi Thorben,

          thank you for the great explanation. Now I got it.
          To be able to understand the three MI classes better I did some smaller refactoring and now I tend to the following solution:

          • add an abstract doExecuteOriginalBehavior() method
          • move
            ```
            if (loopCounter == 0) { innerActivityBehavior.execute(execution); }

            else

            { execution.executeActivity(activity); }

            ```
            into both doExecuteOriginalBehavior() methods

          • wrap the if part for the ParallelMultiInstanceBehavior with try-finally as we talked about before

          I also have another question. In MultiInstanceActivityBehavior there was this part:
          ```
          Object value = null;
          int index = 0;
          Iterator it = collection.iterator();
          while (index <= loopCounter)

          { value = it.next(); index++; }

          ```
          I think that's not a very good way to get the value but maybe you can tell me if I'm wrong.
          An iterator does not guarantee to keep the order of the collection so every newly created iterator could have another order. The MIActivityBehavior is usable with every collection so some strange implementations could be used. Wouldn't it be better to initialize an ArrayList with the collection as constructor argument at the start?
          Does the specification say anything about visiting each element once?

          Cheers,
          Ronny

          Show
          rbraeunlich Ronny Bräunlich added a comment - Hi Thorben, thank you for the great explanation. Now I got it. To be able to understand the three MI classes better I did some smaller refactoring and now I tend to the following solution: add an abstract doExecuteOriginalBehavior() method move ``` if (loopCounter == 0) { innerActivityBehavior.execute(execution); } else { execution.executeActivity(activity); } ``` into both doExecuteOriginalBehavior() methods wrap the if part for the ParallelMultiInstanceBehavior with try-finally as we talked about before I also have another question. In MultiInstanceActivityBehavior there was this part: ``` Object value = null; int index = 0; Iterator it = collection.iterator(); while (index <= loopCounter) { value = it.next(); index++; } ``` I think that's not a very good way to get the value but maybe you can tell me if I'm wrong. An iterator does not guarantee to keep the order of the collection so every newly created iterator could have another order. The MIActivityBehavior is usable with every collection so some strange implementations could be used. Wouldn't it be better to initialize an ArrayList with the collection as constructor argument at the start? Does the specification say anything about visiting each element once? Cheers, Ronny
          Hide
          thorben.lindhauer Thorben Lindhauer added a comment -

          Hi Ronny,

          On the abstract method refactoring:
          Looks alright, however increases the complexity of these classes even further. Perhaps just making the refactoring in MultiInstanceActivityBehavior#executeOriginalBehavior is also ok. It is not necessary in the sequential case but also does no harm if I haven't missed something.

          On the iteration part:
          That is a good point you bring up. However, I am not sure if we should fix this. The ArrayList idea requires that we persist the ArrayList between two transactions, for example in a process variable. Otherwise it would not improve the situation. I am not sure if we should make this change as noone has had issues with this up to now (as far as I know) and, in addition, this would also mean a slight change of behavior since you can also resolve the collection from an expression and therefore change it between two iterations (like adding elements). Don't know though if anyone relies on this

          Cheers,
          Thorben

          Show
          thorben.lindhauer Thorben Lindhauer added a comment - Hi Ronny, On the abstract method refactoring: Looks alright, however increases the complexity of these classes even further. Perhaps just making the refactoring in MultiInstanceActivityBehavior#executeOriginalBehavior is also ok. It is not necessary in the sequential case but also does no harm if I haven't missed something. On the iteration part: That is a good point you bring up. However, I am not sure if we should fix this. The ArrayList idea requires that we persist the ArrayList between two transactions, for example in a process variable. Otherwise it would not improve the situation. I am not sure if we should make this change as noone has had issues with this up to now (as far as I know) and, in addition, this would also mean a slight change of behavior since you can also resolve the collection from an expression and therefore change it between two iterations (like adding elements). Don't know though if anyone relies on this Cheers, Thorben
          Hide
          rbraeunlich Ronny Bräunlich added a comment -

          Hi Thorben,

          I hope I did everything well
          https://github.com/camunda/camunda-bpm-platform/pull/135

          Show
          rbraeunlich Ronny Bräunlich added a comment - Hi Thorben, I hope I did everything well https://github.com/camunda/camunda-bpm-platform/pull/135
          Hide
          thorben.lindhauer Thorben Lindhauer added a comment -

          I'll review in the next days. Thanks Ronny.

          Show
          thorben.lindhauer Thorben Lindhauer added a comment - I'll review in the next days. Thanks Ronny.

            People

            • Assignee:
              thorben.lindhauer Thorben Lindhauer
              Reporter:
              meyer Daniel Meyer
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development