Race condition with raw values

[joachimvh]

Issue type:

• 🐛 Bug

---

Description:

Followup to the discussion in #122 as it is completely independent of the actual PR there and makes more sense as a separate issue.

The problem can be exposed by just adding a random await 'test'; in the code, as having an additional Promise causes the order of operations to change triggering the race condition. Specifically, the last point where adding the Promise causes a problem is this line:

Components.js/lib/construction/argument/ArgumentConstructorHandlerPrimitive.ts

Lines 24 to 25 in 935e509

	// TODO: improve this, so that the hacked valueRaw is not needed
	const rawValue: any = 'valueRaw' in value.term ? (<any> value.term).valueRaw : value.value;

The problem actually occurs in the parsing of JSON literals here:

Components.js/lib/preprocess/parameterproperty/ParameterPropertyHandlerRange.ts

Lines 457 to 458 in ad52da8

	parsed = JSON.parse(value.value);
	(<any>value.term).valueRaw = parsed;

Well, that part and how the RDF object loader works. Since the object loader caches terms that have the same string representation, this means that all literals with the same value will be the same object. Meaning that if you have 2 JSON objects in a literal that are exactly the same, they will be represented by the same Literal object, meaning the code above will write to the same valueRaw parameter twice. This wasn't a problem so far because the Promise resolve order worked out so a value got parsed, the relevant class got instantiated with the necessary JSON input, and only then did the next value get parsed. But if you introduce new Promises in certain places this breaks down and two different classes get instantiated with the same JSON object.

Since we probably don't want to change how the RDF object loader works, the only solution would be to (re-)do the parsing by creating a deep copy in the ArgumentConstructorHandlerPrimitive. Changing the code from that class linked above to the following fixes the problem:

const rawValue: any = 'valueRaw' in value.term ? JSON.parse(JSON.stringify((<any> value.term).valueRaw)) : value.value;

This would cause JSON to be parsed twice but I think the time loss because of that is negligible. You could also remove the parsing from the ParameterPropertyHandlerRange for a minor optimization, but then you would lose the error handling there which might cause other issues.

An alternate solution would be to rework the raw value system as the TODO suggests but that would be more invasive and I don't know what the impact of that would be.

[rubensworks]

Oh, nice debugging work 😲
Deep-copying the valueRaw seems like a good solution to this indeed!
Long-term, it would be better that valueRaw is refactored out, but we'll probably be stuck with it for a long time, so this deep-copying is a good interim solution.

[joachimvh]

Just realized that this doesn't actually solve the problem completely, it just happens to solve the problem in Comunica. It's an edge case and there is probably no actual real use case where this occurs, but assume there are 2 parameters that have the same value "{ "test": "data" }", but one of them has as range xsd:string and the other one has as range xsd:JSON. If the second one gets handled first, valueRaw will be assigned to the literal, which will make the instantiation of the first one incorrect, as that one should still be interpreted as a string and not as JSON. You would need to know what the parameter range is in the ArgumentConstructorHandlerPrimitive to know how to parse the value and guarantee correctness.