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="https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042">https://bugs.eclipse.org/bugs/show_bug.cgi?id=463042</a></li>
+ * <li><a
href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=382308">https://bugs.eclipse.org/bugs/show_bug.cgi?id=382308</a></li>
+ * </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__>_*
*This e-mail may contain trade secrets or privileged, undisclosed, or
otherwise confidential information. If you have received this e-mail in
error, you are hereby notified that any review, copying, or distribution
of it is strictly prohibited. Please inform us immediately and destroy
the original transmittal. Thank you for your cooperation.
_______________________________________________
eclipselink-dev mailing list
eclipselink-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/eclipselink-dev