From 9fe24e6d08f62b66e633c7d32bbabf6d9e6ad45a Mon Sep 17 00:00:00 2001 From: Balmukund Trivedi Date: Tue, 11 Aug 2026 14:25:00 -0700 Subject: [PATCH] Publish the lazily initialised index type caches safely (#4927) MixedIndexTypeWrapper.getFieldKeys builds its ParameterIndexField array on first read and caches it in a field which is not volatile. Nothing orders the writes which fill the array before the write which publishes the reference, so another thread can read a non-null reference to an array whose elements are still null. An array cannot be published safely without a barrier, because its elements are never final. RelationTypeVertex.getKeyIndexes caches the IndexType instances of a property key, so every caller on that vertex is handed the same wrapper. Several threads reach one wrapper when they share a transaction, which a transaction that is not thread bound supports: the vertexCache of StandardJanusGraphTx is concurrent for exactly that reason. The readers are hot paths, IndexSerializer walking the fields on each mutation and the query planner through TypeUtil, and a null element there is a NullPointerException which the source makes look impossible. Make the field volatile, as the three equivalent lazy fields of the superclass IndexTypeWrapper already are. CompositeIndexTypeWrapper caches fields and inlineKeys with the same pattern, so make those volatile too. The race needs a reordering which a given JVM and processor may never perform, so a thread based test would pass whether or not the field is volatile. Assert the property of the fields instead. Signed-off-by: Balmukund Trivedi Co-Authored-By: Claude Opus 5 (1M context) --- .../indextype/CompositeIndexTypeWrapper.java | 4 +- .../indextype/MixedIndexTypeWrapper.java | 2 +- .../IndexTypeWrapperPublicationTest.java | 61 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 janusgraph-core/src/test/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapperPublicationTest.java diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/CompositeIndexTypeWrapper.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/CompositeIndexTypeWrapper.java index 57a82137c0..cfa5ae5a19 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/CompositeIndexTypeWrapper.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/CompositeIndexTypeWrapper.java @@ -35,9 +35,9 @@ */ public class CompositeIndexTypeWrapper extends IndexTypeWrapper implements CompositeIndexType { - private IndexField[] fields = null; + private volatile IndexField[] fields = null; - private String[] inlineKeys = null; + private volatile String[] inlineKeys = null; public CompositeIndexTypeWrapper(SchemaSource base) { super(base); diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java index eb6a295deb..6507de15a4 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java @@ -30,7 +30,7 @@ public class MixedIndexTypeWrapper extends IndexTypeWrapper implements MixedIndexType { public static final String NAME_PREFIX = "extindex"; - private ParameterIndexField[] fields = null; + private volatile ParameterIndexField[] fields = null; public MixedIndexTypeWrapper(SchemaSource base) { super(base); diff --git a/janusgraph-core/src/test/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapperPublicationTest.java b/janusgraph-core/src/test/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapperPublicationTest.java new file mode 100644 index 0000000000..471d153ef2 --- /dev/null +++ b/janusgraph-core/src/test/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapperPublicationTest.java @@ -0,0 +1,61 @@ +// Copyright 2026 JanusGraph Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package org.janusgraph.graphdb.types.indextype; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Modifier; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +//These fields cache a value which is built on first read rather than in the constructor. Nothing orders the writes +//which populate that value before the write which publishes the reference to it, so without volatile another thread +//can observe the reference while the value it points at is still half built. +// +//RelationTypeVertex.getKeyIndexes caches the IndexType instances of a property key, so every caller on that vertex is +//handed the same wrapper. Several threads reach one wrapper when they share a transaction, which a transaction that is +//not thread bound supports: the vertexCache of StandardJanusGraphTx is concurrent for exactly that reason. +// +//The race cannot be reproduced reliably in a test. It needs a reordering which a given JVM and processor may never +//perform, so a thread based test would pass whether or not the field is volatile. This asserts the property itself +//instead. A field which is final is accepted too, so that moving the cache behind an immutable holder does not fail +//here. +public class IndexTypeWrapperPublicationTest { + + @Test + public void mixedIndexFieldsShouldBePublishedSafely() throws Exception { + assertSafelyPublished(MixedIndexTypeWrapper.class, "fields"); + } + + @Test + public void compositeIndexCachesShouldBePublishedSafely() throws Exception { + assertSafelyPublished(CompositeIndexTypeWrapper.class, "fields"); + assertSafelyPublished(CompositeIndexTypeWrapper.class, "inlineKeys"); + } + + @Test + public void sharedIndexTypeCachesShouldBePublishedSafely() throws Exception { + assertSafelyPublished(IndexTypeWrapper.class, "fieldMap"); + assertSafelyPublished(IndexTypeWrapper.class, "cachedTypeConstraint"); + assertSafelyPublished(IndexTypeWrapper.class, "schemaTypeConstraint"); + } + + private static void assertSafelyPublished(Class declaringClass, String fieldName) throws NoSuchFieldException { + final int modifiers = declaringClass.getDeclaredField(fieldName).getModifiers(); + assertTrue(Modifier.isVolatile(modifiers) || Modifier.isFinal(modifiers), + declaringClass.getSimpleName() + "." + fieldName + " can be read by one thread while another writes it, " + + "so it must be volatile or final to be published safely"); + } +}