Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maximum call stack size exceeded in variable resolver #4139

Closed
barmac opened this issue Feb 19, 2024 · 7 comments · Fixed by #4201
Closed

Maximum call stack size exceeded in variable resolver #4139

barmac opened this issue Feb 19, 2024 · 7 comments · Fixed by #4201
Assignees
Labels
bug Something isn't working
Milestone

Comments

@barmac
Copy link
Collaborator

barmac commented Feb 19, 2024

Describe the bug

Sentry still reports RangeError despite bpmn-io/variable-resolver#23

Sentry report

Steps to reproduce

I can't tell.

Expected behavior

No errors.

Environment

  • OS: [e.g. MacOS 10.2, Windows 10]
  • Camunda Modeler Version: 5.20.0
  • Execution Platform: [e.g. Camunda 7, Camunda 8]
  • Installed plug-ins: [...]

Additional context

No response

@barmac barmac added bug Something isn't working ready Ready to be worked on labels Feb 19, 2024
@CatalinaMoisuc
Copy link
Member

We have ~3k events for ~4 users. We should try to root cause the issue before deciding if this should be fixed.

@CatalinaMoisuc
Copy link
Member

CatalinaMoisuc commented Feb 20, 2024

As per @barmac's initial assessment, this issue might be crashing the modeler:

unhandled error in event listener RangeError: Maximum call stack size exceeded

@nikku
Copy link
Member

nikku commented Feb 21, 2024

We have some options:

  • Add additional guardrails to ensure that errors are more comprehensive
  • Add recursion counter and bail out (with proper context) on, i.e. > 100 depth
  • Wait for customer to report a bug with clear steps to reproduce

Leaving to @marstamm to assess further once he is back.

@marstamm
Copy link
Member

marstamm commented Mar 7, 2024

This is indeed another problem than what we fixed previously. What is happening here is that the user has a very big context, like 10k+ entries. We use .push(...arr), which creates a stack for every entry in the array it is adding.

We can prevent this from happening by using different methods of concatenation. What would be interesting to see is if the user actually creates such contexts or if the big amount of entries is a bug.

marstamm added a commit to bpmn-io/variable-resolver that referenced this issue Mar 7, 2024
@marstamm
Copy link
Member

marstamm commented Mar 7, 2024

This one was a doozy:

Background
Let's take a look at the following output mapping:

image

When we resolve output mappings, we create a new interim variable that will be merged at a later point. This ensures that we can correctly handle multiple output mappings with different types and can do the merging in a central space. The interim variables copy all the information from the original variable, so the context information can be displayed in variable suggestions. This is mainly used for nested variables/compositions.

So for the example above, we will have as interim Variables that will be merged:

{
  // Created from Moddle schema
  name: 'foo',
  origin: ['bar'],
  provider: [defaultProvider]
},
{
  // Created from FEEL parsing
  name: 'foo',
  type: 'Number',
  value: 15,

  // copied from original one
  origin: ['bar'], 
  provider: [defaultProvider]
},
{
  // Created from FEEL parsing
  name: 'foo',
  type: 'String',
  value: 'bar,

   // copied from original one
  origin: ['bar'], 
  provider: [defaultProvider]
},

Root Cause
Unfortunately, when merging, we missed to adjust the duplicate checking for also working correctly with providers when we decided to also keep track of variable providers. Therefore, duplicated providers were added to the list.

Because we only copied the reference of the provider array and not the array itself, the size of the array grew exponentially with the number of output mappings.

Together with the spread operator implementation described in #4139 (comment), this leads to a stack size exceeded when you have ~20 mappings for the same variable name.

@nikku
Copy link
Member

nikku commented Mar 14, 2024

Waiting integration, fixed upstream via camunda/camunda-bpmn-js#352.

@nikku nikku added this to the M75 milestone Mar 14, 2024
nikku added a commit that referenced this issue Mar 21, 2024
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed fixed upstream Requires integration of upstream change labels Mar 21, 2024
barmac pushed a commit that referenced this issue Mar 21, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Mar 21, 2024
@barmac
Copy link
Collaborator Author

barmac commented Apr 4, 2024

Related: bpmn-io/bpmn-js-element-templates#78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants