Conversation
bc10d81 to
9fe24e6
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a concurrency safe-publication bug in lazily initialized index-type wrapper caches by ensuring cross-thread visibility of cached array references.
Changes:
- Mark lazily initialized array cache fields as
volatileinMixedIndexTypeWrapperandCompositeIndexTypeWrapper. - Add a JUnit test that enforces safe-publication modifiers (
volatileorfinal) for these shared caches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java | Makes the lazily initialized fields array safely publishable via volatile. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/CompositeIndexTypeWrapper.java | Makes fields and inlineKeys lazy caches safely publishable via volatile. |
| janusgraph-core/src/test/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapperPublicationTest.java | Adds an invariant-style test to prevent regressions in safe publication of lazy caches. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| //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 |
There was a problem hiding this comment.
Fixed in the force-pushed revision, both in the test comment and in the commit message.
| 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"); |
There was a problem hiding this comment.
Good suggestion, applied. The message is now built by a Supplier and ends with the actual modifiers, so a regression reads:
MixedIndexTypeWrapper.fields can be read by one thread while another writes it, so it
must be volatile or final to be published safely, but is declared [private]
I checked that it really does fail that way by dropping the volatile again locally.
) 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 <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-Authored-By: Oleksandr Porunov <alexandr.porunov@gmail.com> Signed-off-by: Oleksandr Porunov <alexandr.porunov@gmail.com>
9fe24e6 to
1e117e7
Compare
|
Reviewed, rebased on current What I checked
On the testI share the mild discomfort about asserting a field modifier by reflection, but I think it earns its place here. The alternative really is a stress test that can pass on a machine which never performs the reordering, which is worse than no test, and without it the fix carries nothing to stop someone dropping the keyword again during a refactor. A field rename makes it fail with Follow-upYour "related, not fixed here" note is correct and I agree it is the more serious of the two: |
Fixes #4927.
MixedIndexTypeWrapper.getFieldKeysbuilds itsParameterIndexField[]on first read and caches it in a field which is notvolatile. 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 is the case where the racy lazy-initialisation idiom cannot be rescued. Final-field semantics give safe publication to immutable objects, but array elements are never final, so the array needs a barrier.
How one wrapper is reached by two threads
JanusGraphSchemaVertex.asIndexType()builds a new wrapper on every call, so the sharing does not come from there. It comes fromRelationTypeVertex.getKeyIndexes(), which caches theIndexTypeinstances 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. A transaction which is not thread bound supports that, and
StandardJanusGraphTxuses aCaffeineVertexCacheso that concurrent threads resolve the same vertex instances. So this needs a multi-threaded transaction rather than ordinary concurrent querying — a supported mode, but not the default one.The readers are hot paths:
IndexSerializer.reindexElementwalks the fields on every mutation and immediately callsfield.getFieldKey(), and the query planner reaches them throughTypeUtil. The array length is always correct, so the symptom is aNullPointerExceptionfrom a null element — an exception the source makes look impossible, because the array is fully built before it is stored.Rarely observed because x86 does not reorder store-store in hardware and the window is one call wide. ARM does permit that reordering, so this is more likely on Graviton or Apple Silicon.
The fix
Make the field
volatile, which is what the three equivalent lazy fields in the superclassIndexTypeWrapperalready do (fieldMap,cachedTypeConstraint,schemaTypeConstraint) using the identical read-into-a-local idiom. No lock is added, so the read stays lock-free and the harmless duplicate construction is preserved.Scope beyond the issue
CompositeIndexTypeWrappercachesfieldsandinlineKeyswith exactly the same pattern, so this PR makes thosevolatileas well. Same defect in the sibling class, one word each. Happy to split it out if you would rather.On the test
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, which is worse than no test.IndexTypeWrapperPublicationTestasserts the property of the fields directly. It acceptsfinalas well asvolatile, so moving a cache behind an immutable holder later does not fail it for the wrong reason.I recognise a reflection assertion on a field modifier is unusual. I chose it over a stress test that cannot fail, but I am happy to drop the test entirely if you would rather not carry it.
Related, not fixed here
RelationTypeVertex.indexesandindexesReferences— the very fields that make these wrappers shared — use the identical pattern, holding aCollections.unmodifiableListover anArrayListwhosesizeand element array are not final. That one may be worse, because a partially visible list yields fewer indexes than exist rather than a null that throws: an index update could be skipped, or the planner could decide no index covers a query, with nothing reported. It is outside this issue, so I left it alone and am glad to open a separate issue for it.For all changes:
master)?For code changes: