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

fix(engine): Skip ByteArray Deletion when Updating Var #4313

Closed
wants to merge 3 commits into from

Conversation

tasso94
Copy link
Member

@tasso94 tasso94 commented May 2, 2024

Problem Description: During Set Variable and Read Variable operations, the following partial ORM operations can occur in a problematic sequence:

  • Thread A reads an Object Variable, thus associated with a byte array value
  • Thread B reads the same Object Variable and updates its value to the db
  • Now Thread A has a leaking byte array reference that is null

Solution: Instead of deleting an updating the byte array value, during the update, the deletion is simply skipped.
The fix was verified with the customer.

The above behaviour introduces leftover byte-array state on type change. Thus, only during a type change, the byte-array is deleted.

Unit-tests: This commit introduces unit tests that verify the correct state of the variables on type change.

Limitations Accepted: The issue could not be reproduced with a unit test. However, this is accepted since the fix follows a common theme for value update across all the project and is not expected to produce a regression. Moreover, the mere reproduction of the problem using private calls doesn't give any extra value.

Co-authored-by: tasso94, petros.savvidis
Related-to: #4324, https://jira.camunda.com/browse/SUPPORT-21494

@tasso94 tasso94 added the ci:all-db Runs the builds for all databases. label May 2, 2024
@tasso94 tasso94 self-assigned this May 2, 2024
@tasso94 tasso94 changed the title fix(engine): don't delete byte array deletion when updating var fix(engine): don't delete byte array when updating var May 2, 2024
@psavidis psavidis force-pushed the SUPPORT-21721 branch 2 times, most recently from 76f1698 to a4320fb Compare June 10, 2024 14:10
@psavidis psavidis added ci:postgresql Runs the builds for the PostgreSQL database. and removed ci:all-db Runs the builds for all databases. labels Jun 10, 2024
@psavidis psavidis changed the title fix(engine): don't delete byte array when updating var fix(engine): Skip ByteArray Deletion when Updating Var Jun 11, 2024
@psavidis psavidis marked this pull request as ready for review June 11, 2024 07:20
@psavidis psavidis force-pushed the SUPPORT-21721 branch 2 times, most recently from d68db41 to ff613da Compare June 11, 2024 07:23
@psavidis psavidis assigned psavidis and tasso94 and unassigned tasso94 Jun 11, 2024
@psavidis
Copy link
Contributor

psavidis commented Jun 11, 2024

✔ The build has already passed with ci:all-db

@psavidis psavidis assigned tasso94 and psavidis and unassigned tasso94 and psavidis Jun 11, 2024
@psavidis psavidis requested review from psavidis and removed request for psavidis June 11, 2024 07:30
@psavidis
Copy link
Contributor

@tasso94 i can't assign this PR to your as a reviewer. Not sure why.

@psavidis psavidis force-pushed the SUPPORT-21721 branch 2 times, most recently from 964c9d4 to 6ed2826 Compare June 12, 2024 08:24
@psavidis
Copy link
Contributor

psavidis commented Jun 12, 2024

Notes for the Reviewer

  • The behaviour to skip deleting the ByteArray introduces the side-effect of having a leftover byte-array state when the type changes e.g from Object to Long
  • To mitigate that problem, the test shouldUpdateVariableStateOnVariableTypeChangeObjectToLong was added to verify that the correct state is populated when the variable has type object, but also when the variable changes to long.
    • That test fails on passes on master because on master, each update will always delete the byte-array
    • On SUPPORT-21721, the test fails without the fix on type change.

Note: The other two tests for Double-to-Object, String-to-Object type changes were added despite the fact that they do not fail on both branches. It is still important to add that coverage for other types

@psavidis psavidis added ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:all-db Runs the builds for all databases. and removed ci:postgresql Runs the builds for the PostgreSQL database. labels Jun 12, 2024
@psavidis psavidis closed this Jun 12, 2024
@psavidis psavidis reopened this Jun 12, 2024
@psavidis psavidis added ci:all-db Runs the builds for all databases. and removed ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:all-db Runs the builds for all databases. labels Jun 12, 2024
@psavidis psavidis closed this Jun 12, 2024
@psavidis
Copy link
Contributor

psavidis commented Jun 12, 2024

Closing this PR in favor of a new one to fix the issue with of being unable to assign the issue to @tasso94 (the PR creator cannot be the reviewer i guess)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-db Runs the builds for all databases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants