Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix @TestTransaction for self-intercepted invocation #16420

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Fix @TestTransaction for self-intercepted invocation #16420

merged 1 commit into from
Apr 12, 2021

Conversation

famod
Copy link
Member

@famod famod commented Apr 10, 2021

This caused quite some confusion in my team as all of our test classes using @TestTransaction were working just fine, but for a single one we were getting something like that (this is actually from TestTransactionTest after I extended it):

[ERROR] io.quarkus.it.panache.TestTransactionTest.test2  Time elapsed: 0.006 s  <<< ERROR!
java.lang.IllegalStateException: BaseTransaction.rollback - ARJUNA016074: no transaction!
        at com.arjuna.ats.internal.jta.transaction.arjunacore.BaseTransaction.rollback(BaseTransaction.java:143)
        at io.quarkus.narayana.jta.runtime.NarayanaJtaProducers_ProducerMethod_userTransaction_c93608e32f9c017c8aafd0401efc2eb68d35aa4e_ClientProxy.rollback(NarayanaJtaProducers_ProducerMethod_userTransaction_c93608e32f9c017c8aafd0401efc2eb68d35aa4e_ClientProxy.zig:197)
        at io.quarkus.narayana.jta.runtime.interceptor.TestTransactionInterceptor.intercept(TestTransactionInterceptor.java:42)
        at io.quarkus.narayana.jta.runtime.interceptor.TestTransactionInterceptorGenerated_Bean.intercept(TestTransactionInterceptorGenerated_Bean.zig:328)
        at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:41)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:41)
        at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:32)
        at io.quarkus.it.panache.TestTransactionTest_Subclass.test2(TestTransactionTest_Subclass.zig:459)
        [...]

which is very confusing because on rollback it has no tx anymore?!

Turns out that by blindly calling rollback(), the TestTransactionInterceptor was shadowing the actual problem:

Caused by: java.lang.IllegalStateException: BaseTransaction.checkTransactionState - ARJUNA016051: thread is already associated with a transaction!
        at com.arjuna.ats.internal.jta.transaction.arjunacore.BaseTransaction.checkTransactionState(BaseTransaction.java:266)
        at com.arjuna.ats.internal.jta.transaction.arjunacore.BaseTransaction.begin(BaseTransaction.java:70)
        ... 106 more

which in turn revealed that the interceptor does not consider self-intercepted invocations of non-private non-test methods from test methods. It was trying to begin a new transaction despite having already done so for the "outer" invocation (the actual test method).

In our case that public helper method was actually an oversight and it should have been private instead. But I don't see why this should be forbidden.
Of course, all this only happens if you use @TestTransaction for the entire test class but IMO that is a good DRY-approach.

TL;DR, this PR:

  • adds a check to avoid any subsequent begin() attempts
  • fixes exception shadowing
  • adds a second test for the "only method annotated case" (I was actually confused why that one test annotated both test class and methods?!)

@quarkus-bot quarkus-bot bot added area/narayana Transactions / Narayana area/panache labels Apr 10, 2021
@famod famod requested a review from stuartwdouglas April 10, 2021 20:26
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 10, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 69069dd

Status Name Step Test failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 10, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building c3231c5

Status Name Step Test failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build ⚠️ Check → Logs Raw logs

@famod
Copy link
Member Author

famod commented Apr 11, 2021

I decided to handle Error as well, mainly so that the interceptor does not shadow AssertionErrors when something goes wrong when rolling back (mostly a theoretical problem, but you never know).

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 11, 2021

Failing Jobs - Building 3370b6f

Status Name Step Test failures Logs Raw logs
Native Tests - Windows - hibernate-validator Build ⚠️ Check → Logs Raw logs

@gsmet gsmet merged commit 610a505 into quarkusio:main Apr 12, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 12, 2021
@famod famod deleted the testtx-selfintercept branch April 12, 2021 08:05
@gsmet gsmet modified the milestones: 2.0 - main, 1.13.2.Final Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/narayana Transactions / Narayana area/panache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants