JanusGraph / JanusGraph/janusgraph
MixedIndexTypeWrapper.fields is a non-volatile lazily-initialised array, allowing unsafe publication
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 5.8k
- Forks
- 1.2k
- Avg merge
- 13h 53m
- Merged PRs (30d)
- 6
Description
- Version: `master` (`ac0eb23`); present in all released versions
- Storage Backend: any
- Mixed Index Backend: any (elasticsearch, solr, lucene)
- Expected Behavior: `MixedIndexTypeWrapper.getFieldKeys()` lazily initialises a shared array and should publish it safely, as the equivalent lazy field in its superclass does.
- Current Behavior: the field is **not** `volatile`, so another thread can observe a non-null array reference whose elements are still null.
### Details
`MixedIndexTypeWrapper` lazily builds and caches `fields`:
https://github.com/JanusGraph/janusgraph/blob/ac0eb2392ddad3d96ab8cde2a5a9a123dbc5d839/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java#L33-L66
```java
private ParameterIndexField[] fields = null; // not volatile
@Override
public ParameterIndexField[] getFieldKeys() {
ParameterIndexField[] result = fields;
if (result==null) {
...
result[pos++]=ParameterIndexField.of(...);
fields = result; // unsafe publication
}
return result;
}
```
The superclass does the same thing correctly — `IndexTypeWrapper.fieldMap` **is** declared `volatile`:
https://github.com/JanusGraph/janusgraph/blob/ac0eb2392ddad3d96ab8cde2a5a9a123dbc5d839/janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/IndexTypeWrapper.java#L39
```java
private volatile Map fieldMap = null;
```
`MixedIndexType` instances are shared across threads via the schema cache, so `getFieldKeys()` is called concurrently. Without `volatile` there is no happens-before edge between the array element writes and another thread's read of `fields`, so a second thread can see a non-null array containing null elements.
Downstream that surfaces as an NPE rather than anything diagnosable, because the only guard is an assertion that is disabled in production:
```java
// IndexRecordUtil.key2Field
public static String key2Field(ParameterIndexField field) {
assert field!=null;
return ParameterType.MAPPED_NAME.findParameter(field.getParameters(), ...); // NPE
}
```
Elasticsearch's provider dereferences `KeyInformation` without null checks in the same way (`ElasticSearchIndex.getAdditionDoc`, `getParameters`), so a torn read during a concurrent schema access can fail a mutation.
### Steps to Reproduce
Hard to reproduce deterministically, as with most publication races. It requires concurrent first-access to the same mixed index from multiple threads, ideally after `resetCache()` (a schema change) has nulled the field on a live graph.
### Suggested Fix
```java
private volatile ParameterIndexField[] fields = null;
```
Matching the superclass. Note that `getFieldKeys()` also returns the internal array directly rather than a copy, so callers can mutate shared schema state; worth considering separately.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in janusgraph-core/src/main/java/org/janusgraph/graphdb/types/indextype/MixedIndexTypeWrapper.java and inspect getFieldKeys(), then compare its fields declaration with IndexTypeWrapper.fieldMap. Make the cached array safely published and verify concurrent first access cannot observe null elements, including after resetCache() on a live graph.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- elasticsearch, java
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 84/100