Discussion: calling a service from inside a JavaDelegate

[jbarrez]

Hi guys,

One thing that is a 'trend' on the forum, is problems sprouting from the fact people try to use one of the services inside their JavaDelegate. The current implementation will simply create a whole new command stack (and new connection/transaction), which leads to strange problems. We've been saying to those people that this is wrong usage … but I can't blame them for assuming it. And so I want to fix this.

I'm talking about the following usage:

public class StartProcessInstanceTestDelegate implements JavaDelegate {

  public void execute(DelegateExecution execution) throws Exception {
    …
    runtimeService.startProcessInstanceByKey("someProcess");
    …
  }

}
‍‍‍‍‍‍‍‍‍‍‍

This can currently go wrong in many ways. For example, what if the process crashes further on, then the process started inside this delegate will have been created (because of the new transaction).

The fix for this issue is actually pretty simple, also because the groundwork in the code was already 99% ready for it: when a new CommandContext is created, it is pushed onto a (threadlocal) stack, and at the end it is popped off again. The solution for our problem here, is to check this stack when a 'nested command' (which is what the service invocation is translated to) is executed. When a context is found on the stack, reuse it. This way, the current transaction is used, and all is how you'd expect it from Activiti and how it handles transactions. The exception is of course the 'txRequiresNew' command stack, which would never want to reuse the context.

I implemented the changes needed in https://github.com/Activiti/Activiti/commit/eba608b2d9023b2a7847e7d1ba819983ca6da882. All tests and Spring test run with these changes. I do need some help from the cdi guys.

You will see I added two test cases in that commit:
* CallServiceInServiceTaskTest.testStartProcessFromDelegate : if you check the logs here, you'll see that the connection is being reused. In the old implementation, you see 2 separate connections.
* CallServiceInServiceTaskTest.testRollBackOnException : this is the interesting one. Here, the behavior will be wrong with the old implementation: one process has an exception, but the other one is still created. In the new implementation, both are rolled back (as you'd expect it).

What do you guys think about this? Do you see any pitfalls I overlooked?

Secondly, you'll see in the test JavaDelegate that I got the runtimeService by calling

RuntimeService runtimeService = Context.getProcessEngineConfiguration().getRuntimeService();
‍‍‍

However, the Context object is an impl object and I'm not thinking about exposing it. As I see it, we can offer two ways to fetch the services:

* Add an API class 'DelegateHelper' which does the call above behind the scenes
* Expose the services on the DelegateExecution

The second approach feels a bit strange to me, but then again people will find it easier….

WDYT?

[trademak]

Hi Joram,

Really good stuff!
I think this definitely solves a lot of issues with building logic that interacts with the Engine in the delegates.
I would think a DelegateHelper would be the most logical approach for providing an entry point to the Activiti interfaces.

Best regards,

[kafeitu]

Hi guys,

One thing that is a 'trend' on the forum, is problems sprouting from the fact people try to use one of the services inside their JavaDelegate. The current implementation will simply create a whole new command stack (and new connection/transaction), which leads to strange problems. We've been saying to those people that this is wrong usage … but I can't blame them for assuming it. And so I want to fix this.

I'm talking about the following usage:


public class StartProcessInstanceTestDelegate implements JavaDelegate {

  public void execute(DelegateExecution execution) throws Exception {
    …
    runtimeService.startProcessInstanceByKey("someProcess");
    …
  }

}

This can currently go wrong in many ways. For example, what if the process crashes further on, then the process started inside this delegate will have been created (because of the new transaction).

The fix for this issue is actually pretty simple, also because the groundwork in the code was already 99% ready for it: when a new CommandContext is created, it is pushed onto a (threadlocal) stack, and at the end it is popped off again. The solution for our problem here, is to check this stack when a 'nested command' (which is what the service invocation is translated to) is executed. When a context is found on the stack, reuse it. This way, the current transaction is used, and all is how you'd expect it from Activiti and how it handles transactions. The exception is of course the 'txRequiresNew' command stack, which would never want to reuse the context.

I implemented the changes needed in https://github.com/Activiti/Activiti/commit/eba608b2d9023b2a7847e7d1ba819983ca6da882. All tests and Spring test run with these changes. I do need some help from the cdi guys.

You will see I added two test cases in that commit:
* CallServiceInServiceTaskTest.testStartProcessFromDelegate : if you check the logs here, you'll see that the connection is being reused. In the old implementation, you see 2 separate connections.
* CallServiceInServiceTaskTest.testRollBackOnException : this is the interesting one. Here, the behavior will be wrong with the old implementation: one process has an exception, but the other one is still created. In the new implementation, both are rolled back (as you'd expect it).

What do you guys think about this? Do you see any pitfalls I overlooked?

Secondly, you'll see in the test JavaDelegate that I got the runtimeService by calling


RuntimeService runtimeService = Context.getProcessEngineConfiguration().getRuntimeService();

However, the Context object is an impl object and I'm not thinking about exposing it. As I see it, we can offer two ways to fetch the services:

* Add an API class 'DelegateHelper' which does the call above behind the scenes
* Expose the services on the DelegateExecution

The second approach feels a bit strange to me, but then again people will find it easier….

WDYT?

I suggest you use spring(IOC).

like this…


@Service
public class StartProcessInstanceTestDelegate implements JavaDelegate {
@Autowired
RuntimeService runtimeService;
    runtimeService.startProcessInstanceByKey("someProcess");
…
}

and set activiti:delegateExpression="${startProcessInstanceTestDelegate}" in foo.bpmn20.xml

[tombaeyens]

From a quick look, it seems like the original StandaloneProcessEngineConfiguration doesn't configure the 'requires' command interceptor stack properly.  It's the same as the requires-new.  And it starts a new command context for every command, which seems to be wrong.

Your solution already introduces an 'if (thereIsATxOngoing()) {joinIt();} else {createNew()}' in the command context interceptor, which is needed to obtain the real semantics of 'requires'.

But I was expecting 2 separate command interceptor stacks in StandaloneProcessEngineConfiguration.  And I don't see that in your solution.  The StandaloneProcessEngineConfiguration still has 2 identical interceptor stacks for requires and requires-new.  That's something I don't understand yet.

[jbarrez]

@ kafeitu: The issue is the same for Spring. In Spring, my test case also fails in the old configuration. I didn't mention it, but that commit also fixes the way you described it, with service injection.

From a quick look, it seems like the original StandaloneProcessEngineConfiguration doesn't configure the 'requires' command interceptor stack properly. It's the same as the requires-new. And it starts a new command context for every command, which seems to be wrong.

Your solution already introduces an 'if (thereIsATxOngoing()) {joinIt();} else {createNew()}' in the command context interceptor, which is needed to obtain the real semantics of 'requires'.

Yes. That is exactly what I fixed.

But I was expecting 2 separate command interceptor stacks in StandaloneProcessEngineConfiguration. And I don't see that in your solution. The StandaloneProcessEngineConfiguration still has 2 identical interceptor stacks for requires and requires-new. That's something I don't understand yet.

The comments in the standalone process engine configuration indicated that this is intented behavior. I traced back history (https://github.com/Activiti/Activiti/blob/master/modules/activiti-engine/src/main/java/org/activiti/... and it is something you introduced 2 years ago. I also didn't understand it.
Hence in my solution, I created two different stacks, one that reuses the context and one that doesn't.

[tombaeyens]

Now I see the (true) and (false) in the StandaloneProcessEngineConfiguration.  I missed those earlier.  That clears things for me.

[meyerd]

The current implementation will simply create a whole new command stack (and new connection/transaction),

I don't think that is entirely true. It really depends on your environment. If you use a thread-scoped transaction & connection propagation strategy like JTA in combination with a correctly operating JCA, you will get the same db connection if you call the activiti service with REQUIRED propagation semantics (default). You will get a new CommandContext & new DbSqlSession, though.
If you use the "standalone" configuration, you will indeed get a new transaction / connection in the inner command which is indeed undesirable in most situations. In that case, you may end up in situations where the "inner" transaction commits but the "outer" transaction rolls back. The success of the inner transaction is also not predicated upon the success of the outer transaction, you could translate a failure of the inner transaction to a transaction rollback of the outer transaction, in this sense it is classical "REQUIRES_NEW".

So currently, the behavior depends on your environment:
* in Standalone mode you have REQUIRES_NEW semantics (new TX & new connection) + NEW DbSqlSession
* in JTA mode you have REQUIRED semantics (same TX + same connection) + NEW DbSqlSession

Joram, in your new approach, you propose the following:
* in Standalone mode you have REQUIRED semantics & SAME DbSqlSession => same TX & same connection
* in JTA mode you have REQUIRED semantics & SAME DbSqlSession => same TX & same connection

If a user does not want REQUIRED semantics, he can use the commandExecutorRequiresNew and get a NEW Transaction + NEW DBsqlSession?

Correct?

[jbarrez]

I don't think that is entirely true. It really depends on your environment. If you use a thread-scoped transaction & connection propagation strategy like JTA in combination with a correctly operating JCA, you will get the same db connection if you call the activiti service with REQUIRED propagation semantics (default). You will get a new CommandContext & new DbSqlSession, though.
If you use the "standalone" configuration, you will indeed get a new transaction / connection in the inner command which is indeed undesirable in most situations. In that case, you may end up in situations where the "inner" transaction commits but the "outer" transaction rolls back. The success of the inner transaction is also not predicated upon the success of the outer transaction, you could translate a failure of the inner transaction to a transaction rollback of the outer transaction, in this sense it is classical "REQUIRES_NEW".

Yes, you are completely correct. IN JTA the situation is indeed better (if you configured it correctly). You still get a new commandcontext though, but that probably doesn't cause much problems.

So currently, the behavior depends on your environment:
* in Standalone mode you have REQUIRES_NEW semantics (new TX & new connection) + NEW DbSqlSession
* in JTA mode you have REQUIRED semantics (same TX + same connection) + NEW DbSqlSession

Correct

Joram, in your new approach, you propose the following:
* in Standalone mode you have REQUIRED semantics & SAME DbSqlSession => same TX & same connection
* in JTA mode you have REQUIRED semantics & SAME DbSqlSession => same TX & same connection

Yes, indeed correct. Note that this only applies to 'nested commands'. There are only a few of that in Activiti (from the top of my head):
- GetNextIdCmd, but it uses the requires-new stack
- DecrementJobRetriesCmd (when an exception occurred. Actually not sure if this one uses requiresNew … but it should)
- etc.

But for the use case I mention above, ie. calling the service from within a delegate, it will use the requires stack.

If a user does not want REQUIRED semantics, he can use the commandExecutorRequiresNew and get a NEW Transaction + NEW DBsqlSession?

Correct?

Yes, indeed. This is a also something on my list: currently, it's pretty hard to fire a custom command (you need some casting). I'd like to see if we can find a better way to expose this, because this is (imho) a very good way to extend the api with 'customer specific' stuff.