Fix NPE on sendCorrelateCommand() side-effect

[remcowesterhoud]

Description

With the engine abstraction topic the response writer is already getting flushed in the executeSideEffects() method of the ProcessingStateMachine. Flushing a second time here causes NullPointerExceptions.

Related issues

closes #9860

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

• The changes are backwards compatibility with previous versions
• If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

• There are unit/integration tests that verify all acceptance criterias of the issue
• New tests are written to ensure backwards compatibility with further versions
• The behavior is tested manually
• The change has been verified by a QA run
• The impact of the changes is verified by a benchmark

Documentation:

• The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
• New content is added to the release announcement
• If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

[github-actions]

Test Results

   804 files  ±    0         1 errors  803 suites  ±0   1h 36m 57s ⏱️ + 4m 38s
6 126 tests +164  6 117 ✔️ +164  9 💤 ±0  0 ❌ ±0 
6 312 runs  +164  6 303 ✔️ +164  9 💤 ±0  0 ❌ ±0 

For more details on these parsing errors, see this check.

Results for commit cea3418. ± Comparison against base commit 77114d6.

♻️ This comment has been updated with latest results.

[korthout]

❌ Please add a test case

[remcowesterhoud]

I don't know how to test this further than the tests we already have. The existing test cases should already cover this as there is no change in functionality.

[korthout]

@remcowesterhoud I don't really understand how the NPE could've happened. But if we do understand, then we can reproduce the situation in a test. The test can assert that the message is correctly correlated. Before your change, the engine would blow up with the NPE. After the change, the test would pass. At least, that's how I think about it, but I didn't really understand the scenario. If you want we discuss it F2F

[remcowesterhoud]

@korthout The NPE that occurred was swallowed and didn't blow up anything. It was an actor that crashed without any impact. Happy to discuss it F2F if it helps.

[korthout]

After discussing F2F, it's clear that the exception is swallowed by the actor. So even if it's reproducible, it's not assertable.

The problem is that any of the side effects could potentially fail (although they shouldn't) and there is no feedback mechanism. Note that we can't roll back the command when we're executing the side effects for it, because some side effects may have already been executed and side effects can't be rolled back.

💡 We could verify that no exceptions are thrown in the side effects execution during tests to avoid this situation in the future. @Zelldon is that something we should consider in the engine abstraction topic?

Anyways LGTM 👍

[remcowesterhoud]

bors merge

[zeebe-bors-camunda]

Build succeeded:

• Java checks
• Test summary
• Go linting