querydsl: QueryDSL not thread-safe, Alias.$(alias.getId()) returns null

I have been using QueryDSL in various projects over the years. Now I have two concrete projects that are struggling with a severe bug issue that QueryDSL: As it seems Alias stuff has a concurrency problem leading to Alias.$ function to return null where it should not. That bug only occurrs in complex multi-threaded batch processing situations and is not so easy to reproduce.

General code pattern of the affected query was something like this (nothing really exciting or exotic):

FooEntity foo = Alias.alias(FooEntity.class);
BarEntity bar = Alias.alias(BarEntity.class);
JPAQuery<FooEntity> query = new JPAQuery<FooEntity>(getEntityManager()).from(Alias.$(foo));
query.join(Alias.$(bar)).on(Alias.$(bar.getFoo().getId()).eq(Alias.$(foo.getId())));
...

We could even reproduce the bug locally and see in the debugger what is going on:

In that situation in debugger we could reproducibly see that Alias.$(foo.getId()) is returning null
We stepped into foo.getId() and saw that the is entering this if: https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/PropertyAccessInvocationHandler.java#L81
It also gets the dummy return value (42) from the propToObj map (used as cache): https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/PropertyAccessInvocationHandler.java#L82
However, the propToExpr map (separate map used as cache for expressions) does not contain the corresponding entry (what seems to be an inconsistent state to me) leading to this putting null in the current ThreadLocal (actually an assertion for not null should be added here producing an exception with additional contextual information): https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/PropertyAccessInvocationHandler.java#L87
To further analyze this, we put a conditional breakpoint in PropertyAccessInvocationHandler with the expression propToExpr.size() != propToObj.size() and came to the conclusion that two different threads where accessing the same CG-Lib proxy instance. As these objects are internally using regular Maps they are not thread-safe and IMHO this is by design.
So we digged our code where we could share Alias CG-Lib proxy objects accross threads. The analysis of our entire code-base gave us the result that we do this nowhere: We always regularily create these alias instances and their sub-objects in local variables and never in members, statics, etc.
Now we digged in QueryDSL and made this observation: https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/Alias.java#L349 This code is calling AliasFactory on a single shared instance: https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/Alias.java#L58 The call leads to this code-line in question: https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/AliasFactory.java#L112 As it seems you are using a cache for these proxy-objects shared accross all threads. https://github.com/querydsl/querydsl/blob/2bf234caf78549813a1e0f44d9c30ecc5ef734e3/querydsl-core/src/main/java/com/querydsl/core/alias/AliasFactory.java#L48 As the Key can easily match accross different threads, I come to the conclusion that QueryDSL is simply not thread-safe and therefore buggy. Any hints where I could be wrong or confirmations of my observation or anything else what could help? Thanks for making QueryDSL and thanks in advance for taking care of this issue.

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 16 (9 by maintainers)

Commits related to this issue

[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago
[#2671] Make Alias.* thread-safe — committed to querydsl/querydsl by jwgmeligmeyling 4 years ago

Most upvoted comments

FYI the issue becomes reproducible after adding the following Rule to AliasTest (making all tests run themselves a couple of times parallel):

public static final int COUNT = 4;

@Rule
public TestRule concurrencyRule = new TestRule() {
    @Override
    public Statement apply(final Statement base, Description description) {
        return new Statement() {
            @Override
            public void evaluate() throws Throwable {
                final CountDownLatch countDownLatch = new CountDownLatch(COUNT);
                ExecutorService executorService = Executors.newFixedThreadPool(COUNT);
                try {
                    List<Future<?>> results = new ArrayList<Future<?>>(COUNT);
                    for (int i = 0; i < COUNT; i++) {
                        results.add(executorService.submit(new Callable<Object>() {
                            @Override
                            public Object call() throws Exception {
                                try {
                                    countDownLatch.countDown();
                                    countDownLatch.await();
                                    base.evaluate();
                                    return null;
                                } catch (Exception e) {
                                    throw e;
                                } catch (Throwable t) {
                                    throw new Exception(t);
                                }
                            }
                        }));
                    }

                    for (Future<?> result : results) {
                        result.get();
                    }
                } finally{
                    executorService.shutdownNow();
                }
            }
        };
    }
};

jwgmeligmeyling on Oct 23, 2020