-
Notifications
You must be signed in to change notification settings - Fork 602
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
Prevent NPE when activating element inside flow scope that's being terminated #11083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @remcowesterhoud LGTM 💯
...io/camunda/zeebe/engine/processing/processinstance/ModifyProcessInstanceTerminationTest.java
Outdated
Show resolved
Hide resolved
final var rejectedActivateCommand = | ||
RecordingExporter.processInstanceRecords(ProcessInstanceIntent.ACTIVATE_ELEMENT) | ||
.withProcessInstanceKey(processInstanceKey) | ||
.withElementId("endEvent") | ||
.onlyCommandRejections() | ||
.getFirst(); | ||
|
||
assertThat(rejectedActivateCommand.getRejectionType()).isEqualTo(RejectionType.INVALID_STATE); | ||
assertThat(rejectedActivateCommand.getRejectionReason()) | ||
.isEqualTo( | ||
"Expected flow scope instance with key '%s' to be present in state but not found.", | ||
eventSubprocessKey); | ||
assertThatElementIsTerminated(processInstanceKey, "eventSubProcess"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Although the result is the same, this doesn't fully align with the expected behavior described in the issue:
I can apply the modification. As a result, the end event should be activated. But the whole event subprocess should be terminated because the activation instruction is applied before the termination instruction.
💭 This would be fixed in when Operate disallows this modification as part of Complex Process Instance Modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is poor wording and it should say that the activate command should be written to the log, as it's an asynchronous action.
…at's being terminated When a modify command contains an activate instruction for an element inside of a subprocess, and also a terminate instruction for the subprocess itself, Zeebe would run into a NullPointer. This is caused by the fact that we keep track of the ancestor keys of an element that's getting activated. We need these to make sure we can clear the interrupted state from any of these ancestors. This is to ensure we support activating elements in an interrupted flow scope. Since we first perform the activate instructions and after this the terminate instructions, we have already terminated the parent of the element that's being activated. We can not get the element instance and potentially remove the interrupted state. A null check needs to be added here.
3e4c6f7
to
8766f41
Compare
bors merge |
Build succeeded: |
Successfully created backport PR #11087 for |
Description
When a modify command contains an activate instruction for an element inside of a subprocess, and also a terminate instruction for the subprocess itself, Zeebe would run into a NullPointer.
This is caused by the fact that we keep track of the ancestor keys of an element that's getting activated. We need these to make sure we can clear the interrupted state from any of these ancestors. This is to ensure we support activating elements in an interrupted flow scope.
Since we first perform the activate instructions and after this the terminate instructions, we have already terminated the parent of the element that's being activated. We can not get the element instance and potentially remove the interrupted state. A null check needs to be added here.
Related issues
closes #10878
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Please refer to our review guidelines.