Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug in VersionSpecificWorkerContextWrapper.allStructureDefinitions() #6661

Open
Boereck opened this issue Jan 29, 2025 · 0 comments · May be fixed by #6766
Open

Bug in VersionSpecificWorkerContextWrapper.allStructureDefinitions() #6661

Boereck opened this issue Jan 29, 2025 · 0 comments · May be fixed by #6766

Comments

@Boereck
Copy link
Contributor

Boereck commented Jan 29, 2025

Describe the bug
The following code on the current (at the time of writing) master:

hapi-fhir/hapi-fhir-validation/src/main/java/org/hl7/fhir/common/hapi/validation/validator/VersionSpecificWorkerContextWrapper.java

Lines 232 to 246 in e412b38

retVal = allStructureDefinitions.stream()
.map(myVersionCanonicalizer::structureDefinitionToCanonical)
.collect(Collectors.toList());
myAllStructures = retVal;
try {
for (IBaseResource next : allStructureDefinitions) {
Resource converted = convertToCanonicalVersionAndGenerateSnapshot(next, false);
retVal.add((StructureDefinition) converted);
}
myAllStructures = retVal;
} catch (Exception e) {
ourLog.error("Failure during snapshot generation", e);
myAllStructures = null;
}

Starting from the try block, the cached StructureDefinitions are snapshotted. However, instead of creating a new List for the resulting snapshotted StructureDefinitions, the resulting Resources are added to the list of previously fetched and converted StructureDefinitions. I am sure this is not correct, because both the non-snapshotted and the snapshotted version are stored to the same list.

In addition to that, the issue causes ConcurrentModificationExceptions, since the List being modified in the try block was stored to the myAllStructures field before. When a second thread calls allStructureDefinitions, before the first one enters the try block, it will get the list and can read it, while the first thread adds elements to that list.

However, this issue is easy to fix: Before entering the try, add the line

retVal = new ArrayList<>();`

@fil512 it seems this was introduced in commit 362dc09 by you. At least you are attributed as the author. Maybe you want to fix the issue, since this is a pretty trivial fix, I can open a PR as well, if this is preferred.

To Reproduce
Steps to reproduce the behavior:
Validate several resources in parallel.
Boereck added a commit to Boereck/hapi-fhir that referenced this issue Mar 3, 2025
@Boereck
Fixing concurrent modification problem in VersionSpecificContextWrapper
6cf0c13 
Snapshotted StructureDefinitions are now added to a new list, instead
of to the list of non-snapshotted StructureDefinitions. This not only
leads to the resource being in the list twice, but also leads to
ConcurrentModificationExceptions when iterating over the result of
allStructureDefinitions().
Fixes hapifhir#6661
@Boereck Boereck linked a pull request Mar 3, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixing concurrent modification problem in VersionSpecificContextWrapper Boereck/hapi-fhir
1 participant
@Boereck