[JENA-2356] Race condition with QueryEngineRegistry and UpdateEngineRegistry init()

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: Jena 4.9.0
• Fix Version/s: Jena 4.10.0
• Component/s: ARQ

Environment

Java 17 on Linux.

• Assignee: Andy Seaborne
• Reporter: Shawn Smith
• Created: 19/Oct/23 18:49
• Updated: 21/Oct/23 13:07
• Resolved: 21/Oct/23 13:06

Description

I encountered this exception in an application where two threads concurrently tried to execute a SPARQL query for the first time since the application started:

 

java.util.ConcurrentModificationException: null
	at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1013)
	at java.base/java.util.ArrayList$Itr.next(ArrayList.java:967)
	at org.apache.jena.sparql.engine.QueryEngineRegistry.find(QueryEngineRegistry.java:94)
	at org.apache.jena.query.QueryExecutionBuilder.build(QueryExecutionBuilder.java:98)
	at org.apache.jena.query.QueryExecutionFactory.make(QueryExecutionFactory.java:622)
	at org.apache.jena.query.QueryExecutionFactory.make(QueryExecutionFactory.java:602)
	at org.apache.jena.query.QueryExecutionFactory.create(QueryExecutionFactory.java:146)
	at org.apache.jena.query.QueryExecutionFactory.create(QueryExecutionFactory.java:158)
        ... 

The application doesn't use QueryEngineRegistry directly in any way, and the relevant code hasn't changed recently. I clearly just got unlucky due to a race condition in how QueryEngineRegistry initializes itself.

At first glance, QueryEngineRegistry initialization looks fine due to the use of a static initializer:

 

public class QueryEngineRegistry
{
    ...
    static { init(); }
    ...
    static QueryEngineRegistry registry = null;

But it turns out that the relative location of the init() and the field declaration matters. Once compiled, the code ^^ is equivalent to this:

 

public class QueryEngineRegistry
{
    ...
    static { init(); }
    ...
    static QueryEngineRegistry registry;
    static { registry = null; } <== UNDOES THE EFFECT OF THE static init()

So the static initializer call to init() ends up having no effect because the field initialization happens afterwards, nulling out the registry created by init().

Instead, initialization falls to the code in the get() method:

 

static public QueryEngineRegistry get()
{
    if ( registry == null )
        init();
    return registry;
}
...
private static synchronized void init()
{
    registry = new QueryEngineRegistry();
    registry.add(QueryEngineMain.getFactory());
    registry.add(QueryEngineFactoryWrapper.get());
}

Despite using synchronized in init(), this initialization strategy is not thread-safe. Once the first line of init() has executed, a concurrent thread that calls get() may receive a partially-initialized instance of QueryEngineRegistry. I believe that is what happened to me to trigger the stack trace above.

 

Fixing the race condition can be as simple as the following change, which fixes the issue with the static initializer:

< static QueryEngineRegistry registry = null;
> static QueryEngineRegistry registry; 

But it seems like a good idea to cleanup the broken initialization code in get(), probably by simply removing it and relying on the static initializer.

And note UpdateEngineRegistry uses the same initialization pattern.

 

Issue Links

• links to: GitHub Pull Request #2047