Bug 1123703 - JTA + SharedEM throwing "IllegalStateException: This persistence strategy only deals with UserTransaction instances!"
Summary: JTA + SharedEM throwing "IllegalStateException: This persistence strategy onl...
Keywords:
Status: VERIFIED
Alias: None
Product: JBoss BPMS Platform 6
Classification: Retired
Component: jBPM Core
Version: 6.0.2
Hardware: Unspecified
OS: Unspecified
high
high
Target Milestone: ER2
: 6.1.0
Assignee: Shelly McGowan
QA Contact: Karel Suta
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-28 03:14 UTC by Josh West
Modified: 2018-12-06 17:29 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: Bug


Attachments (Terms of Use)

Description Josh West 2014-07-28 03:14:27 UTC
Description of problem:

When using Spring, JTA, and a Shared Entity Manager the following exception [4] is thrown when trying to use the audit log service is used. 

Looking into the SpringStandaloneJtaSharedEntityManagerStrategy implementation [1], the illegal state is thrown by a null transaction [2], resulting from the joinTransaction method only returning new transactions [3].

There are currently no tests for the Spring-JTA-SharedEM strategy. 


Version-Release number of selected component (if applicable):
* Found in BPMS 6.0.2GA
* Reproduced in community JBPM 6.2.0-SNAPSHOT

How reproducible:

100%

Steps to Reproduce:
1. Start a transaction - a single transaction typically surrounds a serivce call
2. Create a process instance
3. Attempt to query the process instance from the audit log service

Actual results:

Exception thrown on use of the auditLogService.

Expected results:

AuditLogService usable within the same transaction as the task and runtime service.

Additional info:

[1] https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/SpringStandaloneJtaSharedEntityManagerStrategy.java
[2] https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/StandaloneJtaStrategy.java#L97
[3] https://github.com/droolsjbpm/jbpm/blob/master/jbpm-audit/src/main/java/org/jbpm/process/audit/strategy/StandaloneJtaStrategy.java#L59
[4] java.lang.IllegalStateException: This persistence strategy only deals with UserTransaction instances!
	at org.jbpm.process.audit.strategy.StandaloneJtaStrategy.commitTransaction(StandaloneJtaStrategy.java:98)
	at org.jbpm.process.audit.strategy.SpringStandaloneJtaSharedEntityManagerStrategy.leaveTransaction(SpringStandaloneJtaSharedEntityManagerStrategy.java:29)
	at org.jbpm.process.audit.JPAAuditLogService.closeEntityManager(JPAAuditLogService.java:335)
	at org.jbpm.process.audit.JPAAuditLogService.findProcessInstance(JPAAuditLogService.java:164)
	at org.kie.spring.jbpm.JTASharedEntityManagerFactorySpringTest.testSpringWithJTAAndSharedEMF(JTASharedEntityManagerFactorySpringTest.java:48)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:606)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

Comment 2 Josh West 2014-07-28 03:25:35 UTC
Added test to reproduce issue.  Pull request: https://github.com/droolsjbpm/droolsjbpm-integration/pull/89

Comment 3 Josh West 2014-07-28 04:03:55 UTC
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

Comment 4 Marco Rietveld 2014-07-28 13:17:17 UTC
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).

Comment 5 Maciej Swiderski 2014-07-29 16:23:23 UTC
(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?

Comment 6 Marco Rietveld 2014-07-29 20:53:37 UTC
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.

Comment 7 Maciej Swiderski 2014-08-04 09:35:46 UTC
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?

Comment 8 Marco Rietveld 2014-08-04 14:13:24 UTC
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?

Comment 9 Maciej Swiderski 2014-08-04 14:22:04 UTC
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.

Comment 10 Marco Rietveld 2014-08-04 16:50:00 UTC
Maciej and I talked and figured things out, which was easier than sending messages back and forth and back and forth on the bugzilla. :)

Comment 11 Toshiya Kobayashi 2014-08-29 08:57:37 UTC
The same is reported from a customer who uses AuditLogService in an EJB with TransactionManagementType.CONTAINER.

Comment 13 Karel Suta 2015-01-06 14:58:31 UTC
Verified on 6.1.0 ER3


Note You need to log in before you can comment on or make changes to this bug.