In conjunction with saving XML in commandStack.changed, this module can lead to an infinite loop

[philippfromme]

Describe the Bug

Users might not expect commands to be executed during export and listen to commandStack.changed to call #saveXML which can lead to an unexpected infinite loop.

Example: https://codesandbox.io/s/align-to-origin-bug-hmklo?file=/src/index.js

Steps to Reproduce

1. Listen to commandStack.changed to call #saveXML
2. Trigger a command stack change
3. Infinite loop

Expected Behavior

Either align to origin without the need for a command or somehow make it easier to avoid an unexpected infinite loop (e.g. by defaulting to alignOnSave: false.

Environment

• Library version: v0.6.0

[MaxTru]

This was reported by Cawemo team as well (reported by Andreas Remdt). They actually need to use a workaround to get unblocked

[andreasremdt]

Some additional information: this issue only seems to appear when the last remaining element is deleted from the canvas. If you have any other elements (like tasks, events, etc.) left on the canvas, then it's working as intended. However, if you only have a StartEvent (or Pool) and delete it, the recursion happens.

We also found an issue with the Undo/Redo logic: even with other elements being on the canvas, the deletion of a StartEvent can't be undone by pressing Ctrl+Z. It works for other elements though. Could this be related?

[nikku]

Thanks for providing these additional details #2 (comment) 👏.

We'll have a look and see if we can follow up on this one.

[philippfromme]

After investigating this issue I've come to the conclusion that there's no easy solution.

At the moment the best workaround is to unsubscribe before calling saveXML and subscribe afterwards:

const handleChange = async () => {
  modeler.off("commandStack.changed", handleChange);

  const { xml } = await modeler.saveXML({ format: true });

  modeler.on("commandStack.changed", handleChange);

  console.log(xml);
};

modeler.importXML(xml);

modeler.on("commandStack.changed", handleChange);

Try: https://codesandbox.io/s/bpmn-ioalign-to-origin-workaround-c56yy

Alternatives:

1. Don't use this module 🤡
2. Achieve alignment in a way that doesn't result in a command (e.g. https://github.com/philippfromme/align-to-origin-xml)

Related issues found during the investigation:

1. Module Can Make Undo Impossible
2. EventBus#once Can Lead to Endless Loop
3. Be Able to Avoid Endless Loop When Listening for and Firing Same Event

[nikku]

To conclude my investigation: It is not safe to use commandStack.changed to save your diagram along with this module.

I've sketched a safe way to export with alignOnSave in #5, specifically the AutoSave component is worth having a look.

By design, this module will perform alignment via a command. That has other consequences that make it unsuitable for auto-save: If you undid something, a new alignment command will be added, clearing your undo stack without you noticing. You may have that one on your radar already (https://github.com/camunda/cawemo/issues/6926).

Long story short, do not use this module, at least not with automatic saving on commandStack.changed enabled.

Consider alternatives, i.e. the one sketched by @philippfromme or revise your auto-save strategy. A thought in that direction: Maybe alignment does not need to happen all the time but only once the user actually closes a diagram.

[nikku]

Related to my investigation I added bpmn-io/diagram-js#535 for consideration. A module like auto-save would benefit from command stack compaction. As it is an invisible change for the user, it should be grouped with previous command stack actions and undone/re-done with them.

[nikku]

Related to my investigation I found the following: Given the usage pattern documented above that leads to an infinite loop it can be interesting to see what align-to-origin sees.

By design it does only align if necessary. That means even in recursive commandStack.changed scenarios it would technically be impossible to lead to an infinite loop. align-to-origin aligns once and stops to perform more changes, bringing the recursion to a halt.

In practice however it continues to see that further alignment is needed. As it uses raw DOM APIs under the hood to measure the width and position of elements, it could be that we're not actually querying the correct sizes if alignment and querying happen in quick succession. I did also debug the effect of disabling visual compenstation. I found that one to not have any negative effect.

Attaching a log that visualizes the issue: