| [eclipselink-dev] EclipseLink bugs #463042 and #382308 (ListExpressionOperator instance shared between threads) |
|
Hi all, we’re frequently seeing ArrayOutOfBoundsExceptions or broken SQL generation due to
https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042 and
https://bugs.eclipse.org/bugs/show_bug.cgi?id=382308 in our logs. I have been working on this recently and found that ListExpressionOperator has an internal state (numberOfItems), which gets shared across threads. The affected query is unnamed, has no query hints, hence gets put to
the JPQL parse cache. However, when cloning the query during execution (actually, cloning the DatabaseQueryMechanism), the referenced ListExpressionOperator instance remains shared. Therefore, threads are in ListExpressionOperator.getDatabaseStrings(), while
other threads are modifying numberOfItems simultaneously. ExpressionQueryMechanism.buildBaseSelectionCriteria() clones the selectionCriteria during execution. Therefore, properly cloning the ListExpressionOperator instance in ArgumentListFunctionExpression.postCopyIn() looks
like a remedy. My experiments and tests showed no more shared state between threads and our dynamic JPQL queries run fine so far. Patch is attached to
https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042 (https://bugs.eclipse.org/bugs/attachment.cgi?id=278377) Would you be so kind as to review my changes? It’d be also appreciated to get the changes run through the EclipseLink test suites. Best regards, Patrick On thread http-nio-8080-exec-1: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ EJBQueryImpl.buildEJBQLDatabaseQuery( queryName = null, jpqlQuery = "SELECT E1 FROM Project E1 WHERE E1.uuid IN :projectUuids AND (CASE WHEN (UPPER(E1.name) LIKE CONCAT('%',CONCAT(?1,'%')) ESCAPE
'\') THEN TRUE ELSE FALSE END) = true ORDER BY E1.name", session = ServerSession (id=15932), lockMode = null, hints = null, classLoader = LaunchedURLClassLoader (id=15655) ) databaseQuery=
ReadAllQuery (id=15991) +--queryMechanism JPQLCallQueryMechanism (id=16096) +--selectionCriteria LogicalExpression (id=16112) <=== +--secondChild RelationExpression (id=16117) +--firstChild ArgumentListFunctionExpression (id=16120) +--operator ListExpressionOperator (id=15644)
On thread http-nio-8080-exec-3: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ReadAllQuery.prepareSelectAllRows() (id=16175) +--queryMechanism JPQLCallQueryMechanism (id=16174) +--selectionCriteria LogicalExpression (id=16112) <=== +--secondChild RelationExpression (id=16117) +--firstChild ArgumentListFunctionExpression (id=16120) +--operator ListExpressionOperator (id=15644) 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 6e74c1bbd9..c522df5156 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 @@ -58,7 +58,7 @@ static final long serialVersionUID = -7066100204792043980L; protected int selector; protected String name; - protected String[] databaseStrings; + private String[] databaseStrings; protected boolean isPrefix = false; protected boolean isRepeating = false; protected Class nodeClass; @@ -757,15 +757,24 @@ public boolean conformLike(Object left, Object right) { return true; } - public void copyTo(ExpressionOperator operator){ + /** + * Partial deep-clone of the operator instance. + * <p> + * This is only used by <tt>ListExpressionOperator.copyTo()</tt>. + * + * @param operator + * the operator to copy the current state to + */ + protected void copyTo(ExpressionOperator operator){ + // "name" is not copied. operator.selector = selector; operator.isPrefix = isPrefix; operator.isRepeating = isRepeating; operator.nodeClass = nodeClass; operator.type = type; - operator.databaseStrings = databaseStrings == null ? null : Helper.copyStringArray(databaseStrings); - operator.argumentIndices = argumentIndices == null ? null : Helper.copyIntArray(argumentIndices); - operator.javaStrings = javaStrings == null ? null : Helper.copyStringArray(javaStrings); + operator.databaseStrings = Helper.copyStringArray(databaseStrings); + operator.argumentIndices = Helper.copyIntArray(argumentIndices); + operator.javaStrings = Helper.copyStringArray(javaStrings); operator.isBindingSupported = isBindingSupported; } 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 8f9c9666d0..b2d45b0afe 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 @@ -103,11 +103,22 @@ public void printSQL(ExpressionSQLPrinter printer) { operator.printCollection(getChildren(), printer); } - @Override - protected void postCopyIn(Map alreadyDone) { - ((ListExpressionOperator)operator).setNumberOfItems(0); - Boolean hasLastChildCopy = hasLastChild; + protected void postCopyIn(Map alreadyDone) + { + // The current ArgumentListFunctionExpression just got cloned shallow. + // We need a new operator: as our ListExpressionOperator instance has a + // state, "numberOfItems", it must be cloned not to be shared across + // parallel threads when an unnamed, un-query-hinted, shared, cached + // query is executed. + // This is typical for dynamic JPQL. + final ListExpressionOperator originalOperator = ((ListExpressionOperator) this.operator); + this.operator = new ListExpressionOperator(); + originalOperator.copyTo(this.operator); + + // New operator implicitly initialized to: + // ((ListExpressionOperator) operator).setNumberOfItems(0); + final Boolean hasLastChildCopy = hasLastChild; hasLastChild = null; super.postCopyIn(alreadyDone); hasLastChild = hasLastChildCopy; diff --git a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ListExpressionOperator.java b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ListExpressionOperator.java index 02aec02c43..ef0192e317 100644 --- a/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ListExpressionOperator.java +++ b/foundation/org.eclipse.persistence.core/src/org/eclipse/persistence/expressions/ListExpressionOperator.java @@ -14,6 +14,9 @@ // tware - initial API and implementation from for JPA 2.0 criteria API package org.eclipse.persistence.expressions; +import java.util.Vector; + +import org.eclipse.persistence.internal.expressions.ArgumentListFunctionExpression; import org.eclipse.persistence.internal.helper.Helper; /** @@ -29,26 +32,59 @@ * In the example above "COALESCE(" is the start string, "," is the separator and ")" is the * end string * + * <p> + * <h2>Note</h2> This operator has an internal state, <tt>numberOfItems</tt>, which needs to be considered when caching parsed + * queries. Therefore, {@link ArgumentListFunctionExpression#postCopyIn(java.util.Map)} needs to ensure that a new instance of the + * operator is created when cloning a shared query. + * <ul> + * <li><a href=""> + * <li><a href=""> + * </ul> + * * @see org.eclipse.persistence.internal.expressions.ArgumentListFunctionExpression * @see _expression_#coalesce() * @author tware - * + * @author patrick.haller@xxxxxxx */ public class ListExpressionOperator extends ExpressionOperator { - protected String[] startStrings = null; - protected String[] separators = null; - protected String[] terminationStrings = null; - protected int numberOfItems = 0; - protected boolean isComplete = false; + /** Quasi constant: not modified after initialization of operator instance */ + private String[] startStrings; - @Override - public void copyTo(ExpressionOperator operator){ + /** Quasi constant: not modified after initialization of operator instance */ + private String[] separators; + + /** Quasi constant: not modified after initialization of operator instance */ + private String[] terminationStrings; + + /** volatile: number of items processed by this operator instance */ + private int numberOfItems = 0; + + private boolean isComplete = false; + + /** + * Used to determine whether the database strings array needs to be recalculated, initialized to 0 by JVM + */ + private int cachedNumberOfItems; + + /** + * Cached array of database strings, initialized to null by JVM. + */ + private String[] cachedDatabaseStrings; + + public void copyTo(ExpressionOperator operator) + { + // This has been verified to only operate on new ListExpressionOperator + // instances, hence not susceptible to share volatile state between + // threads. super.copyTo(operator); - if (operator instanceof ListExpressionOperator){ - ((ListExpressionOperator)operator).startStrings = Helper.copyStringArray(startStrings); - ((ListExpressionOperator)operator).separators = Helper.copyStringArray(separators); - ((ListExpressionOperator)operator).terminationStrings = Helper.copyStringArray(terminationStrings); + if (operator instanceof ListExpressionOperator) + { + // Quasi-constant strings for a given SQL operator (CASE, COALESCE, ...) + ((ListExpressionOperator) operator).setStartStrings(Helper.copyStringArray(startStrings)); + ((ListExpressionOperator) operator).setSeparators(Helper.copyStringArray(separators)); + ((ListExpressionOperator) operator).setTerminationStrings(Helper.copyStringArray(terminationStrings)); + // don't copy numberOfItems since this copy method is used to duplicate an operator that // may have a different number of items } @@ -60,25 +96,56 @@ public void copyTo(ExpressionOperator operator){ * case one has been added. */ @Override - public String[] getDatabaseStrings() { - databaseStrings = new String[numberOfItems + 1]; - int i = 0; - while (i < startStrings.length){ - databaseStrings[i] = startStrings[i]; - i++; + public String[] getDatabaseStrings() + { + final int _numberOfItems = numberOfItems; + if (null == cachedDatabaseStrings || cachedNumberOfItems != _numberOfItems) + { + cachedDatabaseStrings = recalculateDatabaseStrings(_numberOfItems); + cachedNumberOfItems = _numberOfItems; } - while (i < numberOfItems - (terminationStrings.length - 1)){ - for (int j=0;j<separators.length;j++){ - databaseStrings[i] = separators[j]; - i++; - } - } - while (i <= numberOfItems){ - for (int j=0;j<terminationStrings.length;j++){ - databaseStrings[i] = terminationStrings[j]; - i++; - } + + return cachedDatabaseStrings; + } + + /** + * Calculate the <i>database strings</i>, based on the <tt>numberOfItems</tt> state. + * + * @return the calculated "database strings", i. e. SQL literals that will be interleaved with expressions, to build the final + * SQL statement in the target database dialect. + */ + private String[] recalculateDatabaseStrings(final int noOfItems) + { + // EJBQueryImpl.buildEJBQLDatabaseQuery() caches + // unnamed, un-query-hinted queries, which is typical for dynamic + // JPQL. This in turn leads to a shared state between threads as + // ArgumentListFunctionExpression's postCopyIn cloning method + // did not clone the ListExpressionOperator, but shared the very + // same instance between multiple threads. + // + // This led to https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042 + // + // The only variables modified from the outside even after initialization + // of a new object instance are: + // - isComplete + // - numberOfItems + + // TODO patrick.haller@xxxxxxx: is this calculation correct? Why is "numberOfItems" including #startStrings and + // #terminationStrings? + final int _numberOfItems = noOfItems - startStrings.length - terminationStrings.length; + assert _numberOfItems > 0; + + final String[] databaseStrings = new String[startStrings.length + _numberOfItems * separators.length + + terminationStrings.length]; + System.arraycopy(startStrings, 0, databaseStrings, 0, startStrings.length); + + int pos = startStrings.length; + for (int j = 0; j < _numberOfItems; j++) + { + System.arraycopy(separators, 0, databaseStrings, pos, separators.length); + pos += separators.length; } + System.arraycopy(terminationStrings, 0, databaseStrings, pos, terminationStrings.length); return databaseStrings; } @@ -90,10 +157,6 @@ public void setNumberOfItems(int numberOfItems){ this.numberOfItems = numberOfItems; } - public String[] getStartStrings() { - return startStrings; - } - public void setStartString(String startString) { this.startStrings = new String[]{startString}; } @@ -114,10 +177,6 @@ public void setSeparators(String[] separators) { this.separators = separators; } - public String[] getTerminationStrings() { - return terminationStrings; - } - public void setTerminationString(String terminationString) { this.terminationStrings = new String[]{terminationString}; } @@ -139,5 +198,15 @@ public boolean isComplete() { return isComplete; } - + /* + * (non-Javadoc) + * @see org.eclipse.persistence.expressions.ExpressionOperator#printsAs(java.util.Vector) + */ + @Override + public void printsAs(Vector dbStrings) + { + // The parent class's side-effect modification of the externalized + // databaseStrings array is unsupported. + throw new UnsupportedOperationException("Use setters exclusively to modify ListExpressionOperator"); + } } Patrick Haller SAP
I
www.sap.com Mandatory Disclosure Statements:
<http://www.sap.com/company/legal/impressum.epx> |