Bug 1123703

Added test to reproduce issue.  Pull request: https://github.com/droolsjbpm/droolsjbpm-integration/pull/89

The issue appears to be caused by the lack of a null check, before checking the transaction object type in `org.jbpm.process.audit.strategy.StandaloneJtaStrategy`. 

When using a shared entity manager and JTA transaction, each service action should not be interacting with the user transaction object (ex: begin/commit). Modifying the type check corrects the issue.

More thorough testing of Spring+JTA+SharedEM is advised. 

[1] https://github.com/shuawest/jbpm/commit/827200856b6f8e2568a6e14b9ffc5802383885f6

It's a little weird that the StandaloneJtaStrategy is being used instead of a strategy for use with spring: 

https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/SpringStandaloneJtaSharedEntityManagerStrategy.java
https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/SpringStandaloneLocalSharedEntityManagerStrategy.java

It looks like this might be a problem with configuration, not with the StandaloneJtaStrategy (which is incorrectly being used here).

(In reply to Marco Rietveld from comment #4)
> It's a little weird that the StandaloneJtaStrategy is being used instead of
> a strategy for use with spring: 
> 
> https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/
> jbpm/process/audit/strategy/SpringStandaloneJtaSharedEntityManagerStrategy.
> java
> https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/
> jbpm/process/audit/strategy/SpringStandaloneLocalSharedEntityManagerStrategy.
> java
> 
> It looks like this might be a problem with configuration, not with the
> StandaloneJtaStrategy (which is incorrectly being used here).

Marco, it seems to be like that because SpringStandaloneJtaSharedEntitManagerStrategy extends StanaloneJtaStrategy and delegates part of the invocation back to the super class.

I believe we should check if the type of transaction after we validate it's not null:
https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/StandaloneJtaStrategy.java#L97

wdyt?

Ah, of course. D'oh.. :/ 

I'm not sure about changing the code in the StandaloneJtaStrategy: the problem that I still see is that the tx is user-controlled (instead of "ksession-controlled") in the use case. In the past, writing code that works in both user-controlled-tx and ksession-controlled-tx situations has not worked out well. 

One of the reasons that I introduced the strategy pattern here is in an effort to make the tx/persistence code here more readable and manageable: by concentrating and separating the code per strategy, we can make sure that each strategy works the way it should. 

In this case, I would much rather introduce a "UserControlledJtaStrategy" or similar class than modify the StandaloneJtaStrategy so that it tries to do 2 things (user-controlled and kiesession-controlled). As I mentioned, doing that is a very slippery slope that in the past has only led to problems. 

What do you think about adding a UserControlledJtaStrategy class? 

The exception that is thrown here should be modified to reflect that the ksession does not have control of the transaction while a strategy is being used that assumes that the ksession *does* have control of the tx.

Marco,

since the main issue with this bz is about AuditLogServie I am not sure we need to have the ksession controlled transaction on the table so to say. Besides clear method on the AuditLogService all other methods are read only so in theory they should not even case about transaction. Although when this service is used within transaction that spans several operation it should be part of the same transaction to be able to see not yet committed operations. 

So what I would suggest is to enhance the StandaloneJtaStrategy to fist make sure that transaction argument is not null before we check if it of given type (UserTransaction) as it might be good enhancement anyway - as I believe that method might be called with null as transaction in many cases (this is based on the further if that checks ut variable) thus checking it up front might be alright.

Agree with your points about the strategy when it comes to audit loggers itself, then it matters if the transaction is ksession controlled or user controlled. So adding the UserControlledJtaStrategy might be worth adding if we see it has a value - currently not sure how would it differ from StandaloneJtaStrategy - maybe besides exception message...

wdyt?

Hi Maciej, 

Unfortunately, I think I didn't write the for the StandaloneJtaStrategy very clearly: 

The code we're talking about is this: 

    protected void commitTransaction(Object transaction) {
        UserTransaction ut = null;
        if( ! (transaction instanceof UserTransaction) ) { 
           throw new IllegalStateException("This persistence strategy only deals with UserTransaction instances!" );
        } else if( transaction != null ){ 
           ut = (UserTransaction) transaction;
        }

This code does exactly what it should do: throw an exception if the submitted "transaction" parameter is not a UserTransaction instance. The "transaction instanceof UserTransaction" line not only tests whether or not the transaction parameter is of the proper class, but it *also tests whether or not the parameter is null*! Because this is the StandaloneJtaStrategy class, the transaction parameter should never be null: code using this strategy should always be able to directly affect the transaction. 

If we add a UserControlledJtaStrategy, the code at this point will be different: it will accept a null parameter. 

My apologies for not explaining this earlier: does my suggestion make more sense to you now?

In general I agree but when looking at the joinTransaction method of StandaloneJtaStrategy you can see that transaction object can be null in case you are already within active transaction and then null will be given to leaveTransaction and in turn into commitTransaction where it will fail as null is not instance of UserTransaction.

More over another part of the code in the commitTransation checks if the ut is not null and then commit as this is explicitly instructing the code that if there is non null transaction it means it was started by the log service and should be committed by the log service as well. Or did I got all wrong now.... sorry for causing confusion.

Maciej and I talked and figured things out, which was easier than sending messages back and forth and back and forth on the bugzilla. :)

The same is reported from a customer who uses AuditLogService in an EJB with TransactionManagementType.CONTAINER.

Fixed (Both Josh's and Toshiya's use cases). Commits; 

6.2.x:
https://github.com/droolsjbpm/jbpm/commit/7ca154c9
https://github.com/droolsjbpm/droolsjbpm-integration/commit/ebb4362b
https://github.com/droolsjbpm/droolsjbpm-integration/commit/c56194fa

master: 
https://github.com/droolsjbpm/jbpm/commit/cb792f7a
https://github.com/droolsjbpm/droolsjbpm-integration/commit/e1dcb98b
https://github.com/droolsjbpm/droolsjbpm-integration/commit/c151ff2d

Verified on 6.1.0 ER3