Project

Profile

Help

Bug #4363

ConcurrentModificationException in XQueryCompiler

Added by Gunther Rademacher about 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Multithreading
Sprint/Milestone:
-
Start date:
2019-10-28
Due date:
% Done:

0%

Estimated time:
Legacy ID:
Applies to branch:
9.9, trunk
Fix Committed on Branch:
9.9, trunk
Fixed in Maintenance Release:
Platforms:

Description

A ConcurrentModificationException occurred when running multiple compilations using the same instance of XQueryCompiler:

java.util.ConcurrentModificationException
	at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:909)
	at java.util.ArrayList$Itr.next(ArrayList.java:859)
	at net.sf.saxon.expr.Assignation.addReference(Assignation.java:403)
	at net.sf.saxon.expr.LocalVariableReference.copy(LocalVariableReference.java:72)
	at net.sf.saxon.expr.InstanceOfExpression.copy(InstanceOfExpression.java:205)
	at net.sf.saxon.expr.OrExpression.copy(OrExpression.java:95)
	at net.sf.saxon.expr.instruct.Choose.copy(Choose.java:566)
	at net.sf.saxon.expr.LetExpression.copy(LetExpression.java:690)
	at com.saxonica.ee.bytecode.ByteCodeCandidate.copy(ByteCodeCandidate.java:113)
	at com.saxonica.ee.optim.OptimizerEE.tryInlineFunctionCall(OptimizerEE.java:1128)
	at net.sf.saxon.expr.UserFunctionCall.optimize(UserFunctionCall.java:406)
	at net.sf.saxon.expr.Operand.optimize(Operand.java:206)
	at net.sf.saxon.expr.Expression.optimizeChildren(Expression.java:617)
	at net.sf.saxon.expr.Expression.optimize(Expression.java:594)
	at net.sf.saxon.ma.arrays.SquareArrayConstructor.optimize(SquareArrayConstructor.java:143)
	at net.sf.saxon.expr.Operand.optimize(Operand.java:206)
	at net.sf.saxon.expr.SlashExpression.optimize(SlashExpression.java:352)
	at net.sf.saxon.expr.flwor.FLWORExpression.optimize(FLWORExpression.java:463)
	at net.sf.saxon.query.XQueryExpression.<init>(XQueryExpression.java:89)
	at com.saxonica.ee.optim.XQueryExpressionEE.<init>(XQueryExpressionEE.java:54)
	at com.saxonica.config.EnterpriseConfiguration.makeXQueryExpression(EnterpriseConfiguration.java:2051)
	at net.sf.saxon.query.XQueryParser.makeXQueryExpression(XQueryParser.java:193)
	at net.sf.saxon.query.StaticQueryContext.compileQuery(StaticQueryContext.java:597)
	at net.sf.saxon.s9api.XQueryCompiler.compile(XQueryCompiler.java:536)
	at Saxon3483.run(Saxon3483.java:71)
	at java.lang.Thread.run(Thread.java:748)

This is reproducible with the test program that is attached to #3483 within a few test cycles, however the exception handling in its run method must be modified in order to avoid NPEs, e.g.:

        public void run() {
                try {
                        getXQueryCompiler().compile(QUERY);
                }
                catch (Exception e) {
                        synchronized (printLock) {
                                e.printStackTrace(System.err);
                                System.err.println("total number of test cycles: " + testCycle);
                                System.exit(1);
                        }
                }
        }

Tested with Saxon-EE 9.9.1.5.

History

#1 Updated by Michael Kay about 2 years ago

I have reproduced the problem (it seems we never "productised" the test case for bug #3483 to make it part of the standard test suite).

The failure occurs while we are inlining a call on m:string() - where the function being inlined is in a precompiled library module. The process of copying the function body appears to be incorrectly making a modification to the original. Specifically, LetExpression.copy() contains the logic

        Expression newAction = getAction().copy(rebindings);
        let.setAction(newAction);
        ExpressionTool.rebindVariableReferences(newAction, this, let);

with rebindings set to null, which means that copying the body (action part) of the LetExpression rebinds the variable references in the new copy to the declaration in the old copy (which adds the copied variable references to the reference list of the original LetExpression); they are then subsequently rebound to the new copy using the rebindVariableReferences() call, but this is too late.

#2 Updated by Michael Kay about 2 years ago

It seems we introduced the RebindingMap passed as an argument to copy() methods way back in Saxon 9.7 in order to prevent this kind of problem, but we never made full use of the mechanism. In fact there are only two paths on which we add variable bindings to the map: in LocalParamBlock, and in PatternThatSetsCurrent.

Changing the code in LetExpression.copy() to add the call rebindings.put(this, let); fixes the problem (this also makes the call in rebindVariableReferences() superfluous, since the rebinding will have been done on the fly).

However, (a) this change will require careful regression testing, and (b) LetExpression is not the only expression affected; the same logic appears in other expressions that bind local variables, for example ForExpression and QuantifiedExpression.

#3 Updated by Michael Kay about 2 years ago

Note the comment in ForExpression.copy():

// TODO: should be able to do this by adding a mapping to rebindings as above. But -s:app-Walmsley -t:d1e41271 crashes.

However, I made the change and ran this test, and it works fine after the change.

#4 Updated by Michael Kay about 2 years ago

I've applied the corresponding change to ForExpression, QuantifiedExpression, and OuterForExpression, and have run the QT3 and XSLT3 test suites, with no apparent regression.

There are a couple of other places where ExpressionTool.rebindVariableReferences() is called where it's less clear what needs to be done:

(a) FLWORExpression.copy() --- this should set up the rebindings when copying the subexpressions, but it's rather more complicated to achieve because we're dealing with multiple variables.

(b) FLWORExpression.rewriteForOrLet() - this doesn't actually copy the subexpressions, it modifies them in situ. In general doing this is not safe, and has been the source of quite a few bugs. But I think in the absence of a test case that reveals a bug, I will leave this code alone. The same applies when we rewrite a ForExpression as a LetExpression in cases where the cardinality of the selection is 1.

(c) OptimizerEE.tryInlineFunctionCall(). Here, given a function such as function f ($m) {$m+1} and a function call f(12), we first copy the function body $m+1 (with an empty Rebinding map); then for each function parameter generate a LetExpression which in this case takes the form let $m := 12, and then, in the copied function body, rebind the variable reference $m to the new LetExpression. This is certainly vulnerable to the same problem that the reference list for the original parameter is being modified. (The inlined function call turns into let $m := 12 return $m+1, which subsequently gets rewritten as let $m := 12 return 13, and finally as 13.)

I think (a) and (c) should probably be addressed.

#5 Updated by Michael Kay about 2 years ago

I've made the change for (a) and it turns out the new code is a bit simpler than the old. Appears to cause no regression.

Regarding (c) the code as written appears to be safe because when a LocalVariableReference is bound to a UserFunctionParameter, no modifications are made to the UserFunctionParameter object. (The object has a referenceCount field, but this is never updated and never used).

#6 Updated by Michael Kay about 2 years ago

  • Category set to Multithreading
  • Status changed from New to Resolved
  • Assignee set to Michael Kay
  • Applies to branch trunk added
  • Fix Committed on Branch 9.9, trunk added

#7 Updated by O'Neil Delpratt about 2 years ago

  • Status changed from Resolved to Closed
  • Fixed in Maintenance Release 9.9.1.6 added

Patch committed to the Saxon 9.9.1.6 maintenance release.

#8 Updated by Gunther Rademacher about 2 years ago

Tested OK with 9.9.1.6. Thanks!

Please register to edit this issue

Also available in: Atom PDF