CommandContext reuse with REQUIRES_NEW propagation inside service task

[stuartchalmers]

Hi,

We have an interesting problem which seems to be related to the introduction of command context reuse in newer versions of Activiti.
I understand the reason this was done, and have read the discussions/blog posts and they make complete sense.

To give some context, we have an existing "retry" business process which is used in some scenarios.
It basically contains a timer event and a service task in a loop (with some other bits and pieces in there).
The service task/Java delegate itself delegates to some other injected event class, and we want this to happen in a new transaction so the  implementation is marked as @Transactional (propagation=REQUIRES_NEW).

This used to mean that we got a new transaction inside the delegate (the transaction can include other DB work and not just Activiti), and on exception this would be rolled back, meanwhile the retry process operating in a different transaction was not and could handle the exception and loop back to the timer, successfully committing its state.
The delegate often itself starts a new Activiti process using RuntimeService. I know this used to be a bad idea in general but as far as I remember for this use case it was fine previously as this process had its own CommandContext / transaction and could be rolled back and the retry loop can continue.

I may have missed something here but as far as I can see, the issue we have now is that even though we are still creating a new transaction inside the delegate, and non-Activiti work is still rolled back, the Activiti work which is done still joins the existing transaction and is not rolled back. This is because the CommandContext is reused, and the DbSqlSession is not flushed until the reused CommandContext is closed when the timer job command completes.

Should/could the CommandInterceptor not check if the current transaction context is the same as it was in the previous command before attempting to reuse it? If not it should still create a new context so that the db changes can be flushed and committed in the new transaction?

Thanks in advance for your thoughts and / or suggestions!

Best Regards
Stuart

[stuartchalmers]

I realised this morning I may be able to work around this by disabling CommandContext reuse in the ProcessEngineConfiguration.
It would be good to hear if anyone has any thoughts on this, and whether it can be considered a bug.
I'm going to try and reproduce this with a simpler test case in the meantime so that I can verify if disabling the reuse works in the way I expect.

[jbarrez]

The CommandContextInterceptor https://github.com/Activiti/Activiti/blob/master/modules/activiti-engine/src/main/java/org/activiti/...  does have a property which you can use to disable this behavior.

Don't let the comment there fool you. The ProcessEngineConfiguration does set it to true. So to fix your problem: subclass the relevant ProcessEngineConfiguration and pass false to that property.

[stuartchalmers]

Thanks Joram,

We've made this change to workaround the issue and are in the process of testing it.

It would be nice if the CommandContextInterceptor could be more intelligent in some way around this as the behaviour had us pretty confused for a while so might impact others, although I guess our use case is rare.

If I get a chance I will look at the source again to see if I can think of any cleaner solution, as we could have other scenarios where the reuse does make sense in the same engine.

Regards
Stuart

[frederikherema1]

Great, keep us posted on your progress…

[marcus1]

This may be related to what I'm working on for ACT-1758.

See GitHub and specifically this commit (WIP).

The idea is that the CommandConfig allows fine-grained control over de CommandInterceptors per command execution (or use the default config).

At the moment the transaction propagation and context reuse can be configured. I think the RetryInterceptor settings are another good candidate for inclusion. This way there is no more need for having multiple interceptor chains per propagation kind. Being able to set whether context reuse is possible should be useful for the issue described in this thread.

[jbarrez]

That looks like an interesting improvement, what's the advantage of splitting the config up?

[marcus1]

For ACT-1758 the schema commands need to run outside of a transaction. There is already the commandExecutorTxRequired and commandExecutorTxRequiresNew, so we could just add another one: commandExecutorTxNotSupported, and use that one for all the schema commands. However, this approach is not very scalable: it is conceivable that other commands need different interceptor combinations and configurations. For example, maybe we want to use the RetryInterceptor only (or except) for certain commands. So do we then add a retryingCommandExecutorTxRequired and a retryingCommandExecutorTxRequiresNew? This would very quickly lead to a unmanagement number or executors.

By decoupling the interceptor configuration from the interceptor stack this problem is avoided. Exactly one executor is sufficient. When passing a command to the executor, the exact configuration can be specified if desired. This allows for very fine grained configuration.

Ofcourse if one does not pass a CommandConfig the default one is used. Therefore most calls to execute can stay the same.

[jbarrez]

Okay - sounds good. We'll check the pull request!

[marcus1]

Great! Let me know if you have any questions and such.