From 6b8c2052b09742424f7de93110dd87356eaee34a Mon Sep 17 00:00:00 2001 From: Igor Mukhin Date: Wed, 12 Jun 2024 12:18:47 +0200 Subject: [PATCH] fixes coalesce and case operators in multithreaded environments (#2136) --- .../expressions/ExpressionOperator.java | 19 ++-- .../ArgumentListFunctionExpression.java | 20 +--- ...mentListFunctionExpressionConcurrency.java | 97 +++++++++++++++++++ 3 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 jpa/eclipselink.jpa.test.jse/src/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ExpressionOperator.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ExpressionOperator.java index f85dd726b6e..ee48ed085b3 100644 --- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ExpressionOperator.java +++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ExpressionOperator.java @@ -2380,15 +2380,12 @@ public void printCollection(List items, ExpressionSQLPrinter printer dbStringIndex = 1; } - if (this.argumentIndices == null) { - this.argumentIndices = new int[items.size()]; - for (int i = 0; i < this.argumentIndices.length; i++){ - this.argumentIndices[i] = i; - } - } + // Empty `this.argumentIndices` means the operator expects a list of arguments with a variable length. + // #2136: As operator's state is shared among all threads, we are not allowed to modify the field `this.argumentIndices`. + int[] argumentIndexes = (this.argumentIndices != null ? this.argumentIndices : arrayIndexSequence(items.size())); String[] dbStrings = getDatabaseStrings(items.size()); - for (final int index : this.argumentIndices) { + for (final int index : argumentIndexes) { Expression item = items.get(index); if ((this.selector == Ref) || ((this.selector == Deref) && (item.isObjectExpression()))) { DatabaseTable alias = ((ObjectExpression)item).aliasForTable(((ObjectExpression)item).getDescriptor().getTables().firstElement()); @@ -2404,6 +2401,14 @@ public void printCollection(List items, ExpressionSQLPrinter printer } } + private int[] arrayIndexSequence(int size) { + int[] result = new int[size]; + for (int i = 0; i < size; i++) { + result[i] = i; + } + return result; + } + /** * INTERNAL: Print the collection onto the SQL stream. */ diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java index 1d43db5429d..b94c9c03a2d 100644 --- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java +++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/internal/expressions/ArgumentListFunctionExpression.java @@ -95,26 +95,14 @@ public void setOperator(ExpressionOperator theOperator) { * Print SQL */ public void printSQL(ExpressionSQLPrinter printer) { - ListExpressionOperator realOperator; - realOperator = (ListExpressionOperator)getPlatformOperator(printer.getPlatform()); - operator.copyTo(realOperator); - ((ListExpressionOperator) realOperator).setIsComplete(true); - realOperator.printCollection(this.children, printer); + ListExpressionOperator operator = (ListExpressionOperator) this.operator; + + operator.setIsComplete(true); + operator.printCollection(this.children, printer); } @Override protected void postCopyIn(Map alreadyDone) { - /* - * Bug 463042: All ArgumentListFunctionExpression instances store the same operator reference. - * Unfortunately, ListExpressionOperator.numberOfItems stores state. If multiple ArgumentListFunctionExpression - * are run concurrently, then the ListExpressionOperator.numberOfItems state shared by all instances - * becomes inconsistent. A solution is to make sure each ArgumentListFunctionExpression has a unique operator - * reference. - */ - final ListExpressionOperator originalOperator = ((ListExpressionOperator) this.operator); - this.operator = new ListExpressionOperator(); - originalOperator.copyTo(this.operator); - Boolean hasLastChildCopy = hasLastChild; hasLastChild = null; super.postCopyIn(alreadyDone); diff --git a/jpa/eclipselink.jpa.test.jse/src/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java b/jpa/eclipselink.jpa.test.jse/src/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java new file mode 100644 index 00000000000..94a63e67f24 --- /dev/null +++ b/jpa/eclipselink.jpa.test.jse/src/org/eclipse/persistence/jpa/test/jpql/TestArgumentListFunctionExpressionConcurrency.java @@ -0,0 +1,97 @@ +package org.eclipse.persistence.jpa.test.jpql; + + +import java.util.ArrayList; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.ObjIntConsumer; + +import javax.persistence.EntityManager; +import javax.persistence.EntityManagerFactory; + +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; + +/** + * This test reproduces the issues #2136, #1867 and #1717. + */ +@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) -> { + var jpql = "SELECT p FROM JPQLEntity p" + + " WHERE p.string1 = coalesce(p.string2, '" + cacheBuster(i) + "')"; + + em.createQuery(jpql, JPQLEntity.class).getResultList(); + }); + } + + @Test + public void testConcurrentUseOfCaseCondition() throws Exception { + runInParallel((em, i) -> { + var jpql = "SELECT p FROM JPQLEntity p" + + " WHERE p.string1 = case when p.string2 = '" + cacheBuster(i) + "' then null else p.string1 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 { + var exception = new AtomicReference(); + + // start all threads + var threads = new ArrayList(); + for (int t = 0; t < MAX_THREADS; t++) { + var thread = new Thread(() -> { + try { + for (int i = 0; i < ITERATIONS_PER_THREAD; i++) { + if (exception.get() != null) { + return; + } + + var em = emf.createEntityManager(); + try { + runnable.accept(em, i); + } finally { + em.close(); + } + + } + } 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(); + } + } +}