Project

Profile

Help

Bug #4218

Multithreading crash - ConcurrentModificationException evaluating subtyping relationship

Added by Michael Kay over 2 years ago. Updated over 2 years ago.

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

100%

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

Description

See bug #4216. During investigation of this a multithreading problem was found. During type checking of a general comparison at GeneralComparison.java#293 we are doing th.isSubtype(t1, NumericType.getInstance()) where t1 happens to be BuiltInAtomicType.STRING. This is leading to a ConcurrentModificationException in TypeHierarchy.java#396 where we call toSet(t2.getPlainMemberTypes()), iterating through the members of the union type :numeric.

History

#1 Updated by Michael Kay over 2 years ago

  • Status changed from New to In Progress

It seems that the synchronization in NumericType.getPlainMemberTypes() isn't strong enough, though I'm not quite sure why.

It works if we make the synchronization stronger, but I think it's probably better to cut the complexity and return a new List each time, rather than trying to memoize it.

#2 Updated by Michael Kay over 2 years ago

Synchronising this method more strongly solves the immediate problem, but I'm now having considerable doubts about the idea that the XPathEvaluator (and similarly the s9api XPathCompiler) are really thread-safe, that is, allowing multiple XPath compilations to take place concurrently.

The problem is that both classes contain an AbstractStaticContext, which contains a KeyManager, and compiling an XPath expression can add key definitions to the KeyManager without synchronization.

#3 Updated by Michael Kay over 2 years ago

The ownership and shareability of the KeyManager look pretty obscure in the code.

For XSLT, there needs to be one KeyManager per compiled package. To this end the KeyManager is held in the PackageData object (which for XSLT will always be a StylesheetPackage). The KeyManager also has a reference to its PackageData object, so it looks like a one-to-one relationship.

So one wonders why the Executable also holds a KeyManager, and why the Controller gets it from the Executable. It looks as if it's only the idref() function that uses the KeyManager held in the Executable; and it looks as if it's only StylesheetPackage that puts a KeyManager in the Executable. So it looks to me as if the idref() function can go straight to getController().getExecutable().getTopLevelPackage().getKeyManager(), and avoid holding the KeyManager redundantly in the Executable.

QueryModule.getKeyManager() contains logic to ensure the Executable's KeyManager is copied into the packageData; we can eliminate that.

So this just leaves XPath; and the question is whether we want to continue ensuring that all XPath expressions compiled under a single XPathCompiler should share the same KeyManager. If we do, we have to make it thread-safe. That's what I've chosen to do.

#4 Updated by Michael Kay over 2 years ago

  • Status changed from In Progress to Resolved
  • Fix Committed on Branch 9.9, trunk added

I believe this has been resolved.

#5 Updated by O'Neil Delpratt over 2 years ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in Maintenance Release 9.9.1.4 added

Bug fix applied in the Saxon 9.9.1.4 maintenance release

Please register to edit this issue

Also available in: Atom PDF