Add bpmn:InclusiveGateway support

[barmac]

Screen.Recording.2022-02-05.at.15.55.52.mov

• Outgoing sequence flow enable/disable
• Default flow handling
• Sync join

Closes #88

[barmac]

Screen.Recording.2022-02-05.at.17.22.55.mov

I've added default flow handling. You cannot select it manually but it will be selected when you disable remaining flows.

[barmac]

Currently, we don't allow to synchronize the tokens. As a result, a converging inclusive gateway is equal to a converging exclusive gateway, i.e. tokens of the same execution don't wait one for another.

I checked how Camunda Platform handles the synchronization (which is enforced) and it seems to count tokens waiting at the gateway and traverse the graph in order to count possible paths to the gateway. If there are no more tokens which can reach the gateway, the execution is resumed. Cf. https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/bpmn/behavior/InclusiveGatewayActivityBehavior.java#L141

If I am to implement it in a similar way (which sounds reasonable), I need to find out how to access the currently active tokens within the execution. Any hints will be appreciated.

[barmac]

Screen.Recording.2022-02-06.at.01.25.38.mov

Now we enforce synchronization on join. It waits for any tokens which can possibly reach the inclusive gateway.

Via a30d5e7 I added Camunda Platform-like behavior that when there are enough tokens waiting at the gateway as the number of incoming flows. I removed it afterwards, because this leads sometimes to a deadlock when there is a fake-join or a parallel gateway is not joined. Not sure if we should account for bad practices. Still, I'd rather avoid deadlock in the simulation when it's possible.

Is it how it's defined in the spec? There's no real specification on the inclusive gateway. Therefore, I can't determine the correct behavior from the PDF.

I will add a couple of test cases later and then I think we can initiate the review.

[barmac]

I've added the following changes:

• Join is now handled as in Camunda Platform, i.e. we wait for at most as many tokens as there are incoming sequence flows; this is similar to how we handle the parallel gateway. If there are no more tokens which have a direct route to the gateway, the join is performed with smaller number of tokens.
• For fork, when user deactivates the last active sequence flow and there is no default flow, another flow is activated. This is to prevent an error when no outgoing flows are active. We could roll this back if there is a way to notify incidents in the simulation.

There is one problem for which we'd need to add more changes to the library. The switch to activate/deactivate a sequence flow is attached to the connection. Then, the context pads module does not allow to set the position of the overlay. As a result, when there are multiple sequence flows starting from the same point, the switches overlap. Have a look at the screenshot:

[barmac]

There is a bug when running multiple simulations at once. We don't properly check the parent context.

Also, we should avoid a non-spec-compliant deadlock when a boundary event is triggered after other tokens arrived.

[nikku]

Moving this back to backlog. Let us await further feedback before actually merging a 🪄 🎉 👾 element behavior.

[barmac]

Sure, let's wait for Gandalf the White to figure this out ^^

[JaySmith]

How can I get this functionality? I would love to have this.

[nikku]

@JaySmith Please refer to #88 (comment).

What is your practical use of inclusive gateway support within this tool?

[barmac]

How can I get this functionality? I would love to have this.

Feel free to fix the remaining issues of this implementation: #110 (comment)

[JaySmith]

@JaySmith Please refer to #88 (comment).

What is your practical use of inclusive gateway support within this tool?

I am documenting our project lifecycle process and at the end our project managers need to perform several activities. For example if they have a open purchase order they need to close it, if they have a capital expenditure request they need to close that as well, if they have implemented a new software solution that needs budget for future maintenance and licensing they need to have that added to the next years budget.

Not all of these will apply. Is there a different way to do that?

Jay

[barmac]

I've fixed the remaining issues. I believe this is now BPMN-compliant.

Screen.Recording.2023-11-20.at.18.20.56.mov

[nikku]

@barmac This looks great! 🤩

Will give it a comprehensive review tomorrow.

[nikku]

Thanks for your contribution. It is great:

• It uses the batteries included facilities the engine already offers (waiting for scopes changing and more ❤️)
• It is really simple at the core
• It even supports default flows in a convenient manner 🎉

---

Please check a bunch of improvements I'd put on top, namely a simplified (and unified) join behavior, and re-using the activityBehavior to advance execution after a join + a bunch of extra tests.

[barmac]

Thanks for the review and the improvements to the PR. I think the changes make sense.

There is still a CI job failing and it complains about CSS file not being present: https://github.com/bpmn-io/bpmn-js-token-simulation/actions/runs/7038213433/job/19154806002?pr=110 I will check if we can fix it easily.

[nikku]

#110 (comment)

This we can ignore, as it is a non-lethal change. In fact the actual test failure is due to a shaky test case:

Unfortunately I was not able to further stabilize the test cases as things work just fine locally for me.

[nikku]

Squashed the changes on this branch (where it makes sense).

[barmac]

I can reproduce the CSS issue but not the test failing. Do we really need to support bpmn-js@8 though? @nikku

[barmac]

Sorry, I missed your comments above. Still, I think we could safely move to bpmn-js@11 or even higher at no cost.

[nikku]

Sorry, I missed your comments above. Still, I think we could safely move to bpmn-js@11 or even higher at no cost.

We're testing against both legacy and new versions, and I think that is fine. Or do you propose to ditch our legacy tests? We'll want to do that at some point, yes.

[barmac]

Given that the legacy test fails for no meaningful reason, the failure is not reproducible locally, and the version the test runs against is 2 years old and 7 major versions behind, my suggestion is to leave v8 behind and test against a version closer to the current one. Or we find out why the tests fail.

[nikku]

@barmac The test failure is non-deterministic, and can happen on the current version of bpmn-js, too.

[nikku]

I dropped the legacy tests via c8bc118, however the build instabilities will likely remain. 👀

[nikku]

Cf. https://github.com/bpmn-io/bpmn-js-token-simulation/actions/runs/7048800536/job/19185774908?pr=110 🙈

[nikku]

Merged, as all test failures are unrelated to this PR.

[barmac]

Oh, that's terrible :(