From d700a2b6d13630760621077cc72885701ada4bb5 Mon Sep 17 00:00:00 2001 From: Igor Mukhin Date: Fri, 13 Sep 2024 10:52:50 +0200 Subject: [PATCH] Fixes coalesce and case operators in multithreaded environments with different number of arguments (backport to 4.0) (#2262) The bug is described in the issue #2136. TestArgumentListFunctionExpressionConcurrency.java - adds tests that reproduce the bug. ArgumentListFunctionExpression.java - fixes printSQL to use the operator itself and not the common instance of it ExpressionOperator.java - fixes getArgumentIndices by disabling the caching of the dynamically created argument indexes. Co-authored-by: Igor Mukhin --- .../expressions/ExpressionOperator.java | 8 +- .../ArgumentListFunctionExpression.java | 7 +- ...mentListFunctionExpressionConcurrency.java | 126 ++++++++++++++++++ 3 files changed, 135 insertions(+), 6 deletions(-) create mode 100644 jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/expressions/ExpressionOperator.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/expressions/ExpressionOperator.java index 71755d9f94b..63bdd8394f5 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/expressions/ExpressionOperator.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/expressions/ExpressionOperator.java @@ -2598,7 +2598,7 @@ public void setArgumentIndices(int[] indices) { } /** - * Return the argumentIndices if set, otherwise initialize argumentIndices to the provided size + * Returns the argumentIndices if set, otherwise returns an array of indexes of the provided size. */ public int[] getArgumentIndices(int size) { int[] indices = this.argumentIndices; @@ -2610,7 +2610,11 @@ public int[] getArgumentIndices(int size) { for (int i = 0; i < indices.length; i++) { indices[i] = i; } - this.argumentIndices = indices; + + // NOTE: Why not cache the newly generated array of indexes like "this.argumentIndices = indices" here? + // The reason is that some operators have variable number of arguments like COALESCE and CASE WHEN. + // As instances of this class are shared between threads we cannot cache the array of indexes. + return indices; } diff --git a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java index 50b5b208306..87c8cd7b05b 100644 --- a/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java +++ b/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2021 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2024 Oracle and/or its affiliates. All rights reserved. * Copyright (c) 2021 IBM Corporation. All rights reserved. * * This program and the accompanying materials are made available under the @@ -94,10 +94,9 @@ public void setOperator(ExpressionOperator theOperator) { */ @Override public void printSQL(ExpressionSQLPrinter printer) { - ListExpressionOperator realOperator; - realOperator = (ListExpressionOperator)getPlatformOperator(printer.getPlatform()); + ListExpressionOperator realOperator = (ListExpressionOperator) getPlatformOperator(printer.getPlatform()); operator.copyTo(realOperator); - ((ListExpressionOperator) realOperator).setIsComplete(true); + realOperator.setIsComplete(true); realOperator.printCollection(this.children, printer); } diff --git a/jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java b/jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java new file mode 100644 index 00000000000..c21bd6c35bc --- /dev/null +++ b/jpa/eclipselink.jpa.test.jse/src/it/java/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java @@ -0,0 +1,126 @@ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024 IBM Corporation. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v. 2.0 which is available at + * http://www.eclipse.org/legal/epl-2.0, + * or the Eclipse Distribution License v. 1.0 which is available at + * http://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: EPL-2.0 OR BSD-3-Clause + */ +package org.eclipse.persistence.jpa.test.jpql; + + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.ObjIntConsumer; + +import org.eclipse.persistence.jpa.test.framework.DDLGen; +import org.eclipse.persistence.jpa.test.framework.Emf; +import org.eclipse.persistence.jpa.test.framework.EmfRunner; +import org.eclipse.persistence.jpa.test.jpql.model.JPQLEntity; +import org.junit.Test; +import org.junit.runner.RunWith; + +import jakarta.persistence.EntityManager; +import jakarta.persistence.EntityManagerFactory; + +/** + * This test reproduces the issues #2136, #1867 and #1717. + * + * @author Igor Mukhin + */ +@RunWith(EmfRunner.class) +public class TestArgumentListFunctionExpressionConcurrency { + + private static final int MAX_THREADS = Math.min(Runtime.getRuntime().availableProcessors(), 4); + private static final int ITERATIONS_PER_THREAD = 1000; + + @Emf(name = "argumentListFunctionExpressionConcurrencyEMF", createTables = DDLGen.DROP_CREATE, classes = { JPQLEntity.class }) + private EntityManagerFactory emf; + + @Test + public void testConcurrentUseOfCoalesce() throws Exception { + runInParallel((em, i) -> { + String jpql; + if (i % 2 == 0) { + jpql = "SELECT p FROM JPQLEntity p WHERE p.string1 = coalesce(p.string2, '" + cacheBuster(i) + "')"; + } else { + jpql = "SELECT p FROM JPQLEntity p WHERE p.string1 = coalesce(p.string2, p.string1, '" + cacheBuster(i) + "')"; + } + em.createQuery(jpql, JPQLEntity.class).getResultList(); + System.out.println(Thread.currentThread().getName() + " - " + i % 2); + }); + } + + @Test + public void testConcurrentUseOfCaseCondition() throws Exception { + runInParallel((em, i) -> { + String jpql; + if (i % 2 == 0) { + jpql = "SELECT p FROM JPQLEntity p" + + " WHERE p.string1 = case " + + " when p.string2 = '" + cacheBuster(i) + "' then p.string1 " + + " else null " + + " end"; + } else { + jpql = "SELECT p FROM JPQLEntity p" + + " WHERE p.string1 = case " + + " when p.string2 = '" + cacheBuster(i) + "' then p.string1" + + " when p.string2 = 'x' then p.string2" + + " else null " + + " end"; + + } + em.createQuery(jpql, JPQLEntity.class).getResultList(); + }); + } + + private static String cacheBuster(Integer i) { + return "cacheBuster." + Thread.currentThread().getName() + "." + i; + } + + private void runInParallel(ObjIntConsumer runnable) throws Exception { + AtomicReference exception = new AtomicReference<>(); + + // start all threads + List threads = new ArrayList<>(); + for (int t = 0; t < MAX_THREADS; t++) { + Thread thread = new Thread(() -> { + try { + for (int i = 0; i < ITERATIONS_PER_THREAD; i++) { + if (exception.get() != null) { + return; + } + + try (EntityManager em = emf.createEntityManager()) { + runnable.accept(em, i); + } + + } + } catch (Exception e) { + exception.set(e); + } + }); + threads.add(thread); + thread.start(); + } + + // wait for all threads to finish + threads.forEach(thread -> { + try { + thread.join(); + } catch (InterruptedException e) { + exception.set(e); + } + }); + + // throw the first exception that occurred + if (exception.get() != null) { + throw exception.get(); + } + } +}