Make @TransactionScoped thread safe (virtual and platform threads)

[imperatorx]

Describe the bug

When a @TransactionScoped bean is accessed inside a transaction for a first time from 2 virtual threads started by the thread that started the transaction (e.g. StructuredTaskScope forks), a race condition can lead to two instances of the bean being created. The transaction context is properly propagated to the virtual threads by smallrye-context-propagation callable wrapping.

Expected behavior

A reentrantlock is expected to be locked before line https://github.com/quarkusio/quarkus/blob/main/extensions/narayana-jta/runtime/src/main/java/io/quarkus/narayana/jta/runtime/context/TransactionContext.java#L110 and released after getting the bean.

Actual behavior

No locking is done in TransactionalContext, thus a race condition can create two beans

How to Reproduce?

1. Define a TransactionScoped bean
2. Create another bean with a method with @Transactional annotation
3. Start 2 virtual threads from that method, e.g. using StrucuredTaskScope.fork(), with proper context propagation (smallrye)
4. From the 2 virtual threads, access a method on the TransactionScoped bean
5. Call the @Transactional method

Repeat a lot of times and watch for multiple bean creation per method call.

@Inject TransactionScopedBean myBean;

@Transactional
public void foo() {
  try(var scope = new StrucuredTaskScope.ShutdownOnFailure()) {
    scope.fork(wrapWithContext(()->myBean.something())); // race condition may occur here
    scope.fork(wrapWithContext(()->myBean.something())); // race condition may occur here
    scope.join();
  }
}

Output of uname -a or ver

all platforms

Output of java -version

19

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.13.3.Final

Build tool (ie. output of mvnw --version or gradlew --version)

all build tools

Additional information

No response

[geoand]

cc @anavarr @cescoffier

[imperatorx]

Here is a single file reproducer, it happens in platform threads and virtual threads too. The key point is the expensive resoruce in the producer. Each request will create 4 instances of the transactionscoped bean instead of 1. If the Thread.sleep(100) is removed, the chance of error is smaller.

package org.acme.reproducer;

import org.eclipse.microprofile.context.ThreadContext;

import javax.inject.Inject;
import javax.inject.Singleton;
import javax.transaction.TransactionScoped;
import javax.transaction.Transactional;
import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

@Path("/hello")
public class GreetingResource {

    @Inject
    ThreadContext threadContext;

    @Inject
    MyTransactionalBean myTransactionalBean;

    private final ExecutorService someExecutor = Executors.newCachedThreadPool();

    @GET
    @Produces(MediaType.TEXT_PLAIN)
    @Transactional
    public String hello() {

        System.err.println("Begin transaction");

        // Call transaction bean 4 times
        var f1 = someExecutor.submit(threadContext.contextualRunnable(() -> myTransactionalBean.doWork()));
        var f2 = someExecutor.submit(threadContext.contextualRunnable(() -> myTransactionalBean.doWork()));
        var f3 = someExecutor.submit(threadContext.contextualRunnable(() -> myTransactionalBean.doWork()));
        var f4 = someExecutor.submit(threadContext.contextualRunnable(() -> myTransactionalBean.doWork()));

        try {
            f1.get();
            f2.get();
            f3.get();
            f4.get();
        } catch (Exception e) {
            throw new RuntimeException(e);
        }

        System.err.println("End transaction");

        return "Hello from RESTEasy Reactive";
    }

    @Singleton
    public static class MyBeanProducer {

        @javax.enterprise.inject.Produces
        @TransactionScoped
        public MyTransactionalBean bean() {
            System.err.println("Creating a bean instance! Only one should exist per transaction");
            try {
                // Simulate acquire expensive resource
                Thread.sleep(100); // Cause race condition
            } catch (Exception e) {

            }
            return new MyTransactionalBean();
        }
    }

    public static class MyTransactionalBean {
        public void doWork() {
            try {
                Thread.sleep(50);
            } catch (Exception e) {

            }
        }
    }
}

Output:

Begin transaction
Creating a bean instance! Only one should exist per transaction
Creating a bean instance! Only one should exist per transaction
Creating a bean instance! Only one should exist per transaction
Creating a bean instance! Only one should exist per transaction
End transaction

[mkouba]

I'm not quite sure that @TransactionScoped beans and TransactionContext are designed to be used concurrently. Also I don't think you can use a JTA transaction in parallel. AFAIK the main motivation of MP Context Propagation is to propagate contexts in a reactive pipeline.

CC @FroMage @manovotn @mmusgrov

[imperatorx]

My use case is to start a JTA transaction, consume a message from an MQ and then create multiple virtual threads which utilize non-jdbc, but XA capable resources, e.g. a custom FoundationDB resource that is enlisted to the JTA transaction, then commit the whole transaction in an atomic unit. This custom FoundationDB resource supports concurrent access from multiple threads, and is enlisted as an LRCO item in the 2P commit.

I would agree that for regular JDBC this is a non-issue since you use one thread, but it would be nice if users could use their cutom XA resources from multiple virtual threads, but of course this can also be implemented in a custom CDI scope which does the locking.

[mkouba]

Hm, it seems that the MP CP spec supports both variants, i.e. Propagation of Active Transactions for Serial Use, but not Parallel and Propagation of Active Transactions for Parallel Use. I'm not sure which variant is supported by Naryana/Quarkus.

[mkouba]

I would agree that for regular JDBC this is a non-issue since you use one thread, but it would be nice if users could use their cutom XA resources from multiple virtual threads, but of course this can also be implemented in a custom CDI scope which does the locking.

Note that you would need to make the @TransactionScoped beans thread-safe as well...

[manovotn]

Hm, it seems that the MP CP spec supports both variants, i.e. Propagation of Active Transactions for Serial Use, but not Parallel and Propagation of Active Transactions for Parallel Use. I'm not sure which variant is supported by Naryana/Quarkus.

I should note that the spec only guarantees the non-parallel version to work.
The parallel variant is defined as optional on a per-impl basis.
Now, whether it should or shouldn't work (in which case there should be an exception?!) is mostly up to @mmusgrov to asnwer.

The impl in Quarkus uses Narayana but some of the code here is copied over and/or slightly altered.
However, even directly in Narayana impl, this aspect seems to match what we have in Quarkus, see this code.

[mmusgrov]

In the JTA specification the transaction can only be shared between two threads if is suspended on one thread and resumed on the other.

Some implementatations of MicroProfile Context Propagation do support sharing the context [1] and, behind the scenes, the narayana/Smallrye implementation [2] does the suspend and resume management. But to use this feature you would need use the MP Context Propagation APIs. The narayana-jta quarkus extension is pulling in that dependency [3].

[1] https://download.eclipse.org/microprofile/microprofile-context-propagation-1.0/microprofile-context-propagation.html#_propagation_of_active_transactions_for_serial_use_but_not_parallel
[2] https://github.com/smallrye/smallrye-context-propagation/blob/main/jta/src/main/java/io/smallrye/context/jta/context/propagation/JtaContextProvider.java
[3] https://github.com/quarkusio/quarkus/blob/main/extensions/narayana-jta/runtime/pom.xml#L34

[mmusgrov]

So the MP CP providers are present in the JTA quarkus extension but I don't know where the tests would be - is their a test suite that checks quarkus for MP compliance (since the compliance TCK has plenty tests in this area)?

[manovotn]

@mmusgrov MP TCKs are all here - https://github.com/quarkusio/quarkus/tree/main/tcks/microprofile-context-propagation
I have no idea how much of the JTA stuff is there because IIRC, some of those tests were optional (since,as stated here, parallel transaction may or may nor be supported, depending on the impl)

[mmusgrov]

The optionality, if that's a real word, is in the implementation of the spec - the smallrye-jta one implements the option.

[mmusgrov]

@manovotn it's been a while since I looked (the first smallrye implementation of the spec definitely tested parallel use of JTA transactions using the narayana based implementation) so I am checking now to verify that my statement is still accurate ...

[mmusgrov]

The test for the feature is marked @Ignore in the MP CP TCK. Since it is no longer being tested in the TCK I don't think you can necessarily rely on it still working in the Smallrye implementation, i.e. we would have to develop our own test for it, perhaps based on the one in the TCK (see ref [1] below).

I asked for clarification over in the https://gitter.im/eclipse/microprofile-concurrency room. I couldn't find a way to link to the chat so, at risk of cross posting, the question I asked was:

Hi @njr-11 I wasn't sure where to ask a transaction context propagation question so I thought here would be a good place to start.

I was looking through the TCK to see what the status of testing ConcurrentTransactionPropagation for transaction scoped beans was. I noticed that the test is still @Ignored.

I do recall that we originally added the Ignore because we hadn't decided on "how to signal lack of support for propagation to multiple threads in parallel".

The chapter in the spec on "Propagation of Active Transactions for Parallel Use" [2] says that the behaviour is optional.

My question is how does the user determine whether or not the option is supported and how would the TCK verify that if it was?

[1] https://github.com/eclipse/microprofile-context-propagation/blob/master/tck/src/main/java/org/eclipse/microprofile/context/tck/cdi/JTACDITest.java#L250
[2] https://download.eclipse.org/microprofile/microprofile-context-propagation-1.0/microprofile-context-propagation.html#_propagation_of_active_transactions_for_parallel_use

[mmusgrov]

The answer I got is that an implementation should raise IllegalStateException if the feature is unsupported and therefore the TCK cannot test compliance. So we'd need to add our own tests if we want to support this behaviour.