Activiti
  1. Activiti
  2. ACT-482

Bug in nested fork/join: failing to pass through two consective joins

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.0
    • Fix Version/s: 5.2
    • Component/s: Engine
    • Labels:
      None
    • Environment:
    • Testcase included:
      yes
    • Number of attachments :
      2

      Description

      Following is the trace of example process (see ForkJoin.png and full example):

      ConcurrentExecution[6] takes transition (Task_A)--Flow_4-->(Join_AB)
      ConcurrentExecution[6] executes Activity(Join_AB): org.activiti.engine.impl.bpmn.ParallelGatewayActivity
      inactive concurrent executions in 'Activity(Join_AB)': [ConcurrentExecution[6]]
      parallel gateway 'Join_AB' does not activate: 1 of 2 joined
      transitions to take concurrent: [(Join_B)--Flow_3-->(Join_AB)]
      ProcessInstance[3] takes transition (Join_B)--Flow_3-->(Join_AB)
      ProcessInstance[3] executes Activity(Join_AB): org.activiti.engine.impl.bpmn.ParallelGatewayActivity
      inactive concurrent executions in 'Activity(Join_AB)': [ProcessInstance[3]]
      parallel gateway 'Join_AB' does not activate: 1 of 2 joined

      For some reason Join_AB is reached twice, but the second call does not pass through the join, but logging 1 of 2 joined for the second time. It should discover a second execution reaching the join and pass further to Task_C, which does not happen.

      However, if the process is slightly modified so that sequence flows from Task B1 and Task B2 end up directly in Join AB (now it joins 3 executions), the process goes further breaking the join as expected:

       ConcurrentExecution[6] takes transition (Task_A)--Flow_4-->(Join_AB)
       ConcurrentExecution[6] executes Activity(Join_AB): org.activiti.engine.impl.bpmn.ParallelGatewayActivity
       inactive concurrent executions in 'Activity(Join_AB)': [ConcurrentExecution[6]]
       parallel gateway 'Join_AB' does not activate: 1 of 3 joined
       ConcurrentExecution[7] takes transition (Task_B1)--Flow_11-->(Join_AB)
       ConcurrentExecution[7] executes Activity(Join_AB): org.activiti.engine.impl.bpmn.ParallelGatewayActivity
       inactive concurrent executions in 'Activity(Join_AB)': [ConcurrentExecution[6], ConcurrentExecution[7]]
       parallel gateway 'Join_AB' does not activate: 2 of 3 joined
       ConcurrentExecution[12] takes transition (Task_B2)--Flow_10-->(Join_AB)
       ConcurrentExecution[12] executes Activity(Join_AB): org.activiti.engine.impl.bpmn.ParallelGatewayActivity
       inactive concurrent executions in 'Activity(Join_AB)': [ConcurrentExecution[6], ConcurrentExecution[12], ConcurrentExecution[7]]
       parallel gateway 'Join_AB' activates: 3 of 3 joined
       transitions to take concurrent: [(Join_AB)--Flow_8-->(Task_C)]
       ProcessInstance[3] takes transition (Join_AB)--Flow_8-->(Task_C)

      Full failing test case (branch join-bug): https://github.com/nurkiewicz/try-ipad2/tree/join-bug
      Passing testcase (branch join-bug-workaround): https://github.com/nurkiewicz/try-ipad2/tree/join-bug-workaround
      (only after applying this change: https://github.com/nurkiewicz/try-ipad2/commit/65432c98b1ace4a4cdd60dc4aaee453f4cf09b80)

      1. ACT-482.patch
        2 kB
        Tomasz Nurkiewicz
      1. ForkJoin.png
        30 kB

        Activity

        Hide
        Joram Barrez added a comment -

        I verified the process, and indeed it is a (huge) bug.

        I committed the test + fix in revision 1817
        See Fisheye: http://fisheye.codehaus.org/changelog/activiti/?cs=1817

        Test: ParallelGatewayTest.testNestedForkJoin
        Fix: in ExecutionEntity

        boolean allInSameActivity = true;
        if (concurrentInActiveExecutions.size() > 1) {
        String activityId = concurrentInActiveExecutions.get(0).getActivityId();
        for (ExecutionEntity execution: concurrentInActiveExecutions) {
        if (!execution.isEnded && !execution.getActivityId().equals(activityId))

        { allInSameActivity = false; }

        }
        }

        However, I'm not confident by this quick-fix.
        The problem was that inactive executions were determinded for a certain concurrentRoot. In case there arent any active concurrent executions, the concurrentRoot is used to continue the process. However, I believe this is only valid in case that the concurrent executions are at the same activity (which they werent in the process attached to this issue).

        Tom, could you review/improve it?

        Show
        Joram Barrez added a comment - I verified the process, and indeed it is a (huge) bug. I committed the test + fix in revision 1817 See Fisheye: http://fisheye.codehaus.org/changelog/activiti/?cs=1817 Test: ParallelGatewayTest.testNestedForkJoin Fix: in ExecutionEntity boolean allInSameActivity = true; if (concurrentInActiveExecutions.size() > 1) { String activityId = concurrentInActiveExecutions.get(0).getActivityId(); for (ExecutionEntity execution: concurrentInActiveExecutions) { if (!execution.isEnded && !execution.getActivityId().equals(activityId)) { allInSameActivity = false; } } } However, I'm not confident by this quick-fix. The problem was that inactive executions were determinded for a certain concurrentRoot. In case there arent any active concurrent executions, the concurrentRoot is used to continue the process. However, I believe this is only valid in case that the concurrent executions are at the same activity (which they werent in the process attached to this issue). Tom, could you review/improve it?
        Hide
        Tomasz Nurkiewicz added a comment -

        I took a look at your fix, I can only suggest some small refactoring to improve code readability (attached). But still the condition:

            if((transitions.size()==1)
                 && (concurrentActiveExecutions.isEmpty())
                 && allExecutionsInSameActivity(concurrentInactiveExecutions)
               )
        

        looks too complicated and I guess it will grow in time dangerously. Probably the whole routine should be reviewed and rewritten to make it easier to maintain in the future. Nevertheless thank you for the fix!

        Show
        Tomasz Nurkiewicz added a comment - I took a look at your fix, I can only suggest some small refactoring to improve code readability (attached). But still the condition: if ((transitions.size()==1) && (concurrentActiveExecutions.isEmpty()) && allExecutionsInSameActivity(concurrentInactiveExecutions) ) looks too complicated and I guess it will grow in time dangerously. Probably the whole routine should be reviewed and rewritten to make it easier to maintain in the future. Nevertheless thank you for the fix!

          People

          • Assignee:
            Tom Baeyens
            Reporter:
            Tomasz Nurkiewicz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: