Closed joachimvh closed 1 year ago
coveralls
commented
1 year ago | Totals | |
|---|---|
| Change from base Build 5056899873: | 0.0% |
| Covered Lines: | 1365 |
| Relevant Lines: | 1365 |
rubensworks
commented
1 year ago If you can provide me with what exactly with the commands that need to be tested on Comunica I can look into it.
If both yarn run build and bin/query-dynamic.js from query-sparql work, then everything in Comunica should be fine.
joachimvh
commented
1 year ago That seems to still generate an error so will have to investigate further.
joachimvh
commented
1 year ago Found out where the error happens, but I actually have no idea why the changes to the ArgumentConstructorHandlerHash cause it to happen though.
The error is triggered by the code interaction here: https://github.com/comunica/comunica/blob/aa206ee718ac27f8c9caa508e321db41f2ca3349/packages/actor-abstract-mediatyped/lib/ActorAbstractMediaTypedFixed.ts#L13-L18
At some point the ActorAbstractMediaTypedFixed constructor gets called with a mediaTypePriorities object that was already used previously for such a constructor. That means that in the previous call that object got frozen, so trying to assign values to it the second time causes an error to be thrown.
Not sure if relevant, but the object that it happens with is the default value of ActorRdfSerializeN3: https://github.com/comunica/comunica/blob/7e5470d52489297158ad74624499416add214b3b/packages/actor-rdf-serialize-n3/lib/ActorRdfSerializeN3.ts#L16-L22
I have no idea why though. Is it an error that causes the Components.js to try and instantiate the same object twice, or are there 2 separate objects that take the same input and Components.js reuses that object somehow? I can't see a link between the changes in ArgumentConstructorHandlerHash and this error suddenly occurring.
rubensworks
commented
1 year ago I have no idea why though. Is it an error that causes the Components.js to try and instantiate the same object twice, or are there 2 separate objects that take the same input and Components.js reuses that object somehow? I can't see a link between the changes in ArgumentConstructorHandlerHash and this error suddenly occurring.
A possible next step would be to check if the same object really gets instantiated twice.
Object.id may help to see if two objects are identical.
Does the problem go away if Object.freeze is removed?
joachimvh
commented
1 year ago Did some more checks, it's not the same object being instantiated twice. One case is the ActorRdfSerializeN3 while the other one is the ActorRdfParseN3. They both define a default value for the mediaTypePriorities parameter which happens to be the same, and the change here somehow causes one object to be used in both cases.
Removing the freeze does solve the problem, but then that still means you have an object being reused which is something you don't want. So it's actually quite accidental this issue got discovered.
rubensworks
commented
1 year ago They both define a default value for the mediaTypePriorities parameter which happens to be the same,
But isn't this exactly the problem then? For some reason, this change makes it so that the mediaTypePriorities parameter value is reused for different instances, while it shouldn't be.
joachimvh
commented
1 year ago Yea, the problem is that I don't know why the code change here causes that to happen.
Just calling argsCreator.getArgumentValues on the key causes the issue, even if all the rest of the code remains the same. And as far as I can tell this is the call stack when calling that function:
ConfigConstructor.getArgumentValuesConfigConstructor.getArgumentValueArgumentConstructorHandlerPrimitive.handleConstructionStrategyCommonJs.createPrimitiveNone of these functions change any kind of state from what I can see so it's a mystery why just adding that call there causes behaviour to change. Clearly there's a reason but I can't find it 😄.
joachimvh
commented
1 year ago Just to add on to how weird this issue is. If you undo this PR and use the current Components.js, you can reproduce this error by adding the following line to the beginning of the handle function of the ArgumentConstructorHandlerHash
await argsCreator.getArgumentValue(argsCreator.objectLoader.createCompactedResource('"test"'), {});If you add this line to the end of the function instead, right before returning the result, this error will not occur.
Reducing the above to the functions that are actually called you can actually even do it with this line:
await argsCreator.constructionStrategy.createPrimitive({ settings, value: 'test' });createPrimitive is a function that just returns a string so nothing should happen there. If you remove the await (which should also not have an impact), the code does not result in the above problem. At this point it somehow seems more likely there is just something wrong with my test setup but I did make sure to have clean installed dependencies before trying this.
rubensworks
commented
1 year ago That's weird 😅
Have you checked if mediaTypePriorities that comunica receives is a plain object, and not something special?
What happens if you make a deep copy of it before the forEach?
rubensworks
commented
1 year ago @joachimvh I guess it's safe to merge and release this one now? Or are more changes needed here?
joachimvh
commented
1 year ago This one is fine now.
rubensworks
commented
1 year ago Super! Released as 5.4.2.
coveralls
commented
2 months ago This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
| Totals | |
|---|---|
| Change from base Build 5056899873: | 0.0% |
| Covered Lines: | 1365 |
| Relevant Lines: | 1365 |
Second attempt, see https://github.com/LinkedSoftwareDependencies/Components.js/pull/121
If you can provide me with what exactly with the commands that need to be tested on Comunica I can look into it.