You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@jena.apache.org by "Ostrzyciel (via GitHub)" <gi...@apache.org> on 2024/10/26 09:32:15 UTC
[I] Race condition in TypeMapper.getSafeTypeByName [jena]
Ostrzyciel opened a new issue, #2795:
URL: https://github.com/apache/jena/issues/2795
### Version
5.2.0
### What happened?
I've been chasing down the reasons for a flaky test in (you guessed it) a scalatest suite. The test was failing randomly and seemingly for no reason on isomorphism checks of two graphs that should be identical.
After some debugging, I've discovered the following:
- The root cause is comparing literals with non-standard datatypes (i.e., datatypes not preloaded on Jena startup).
- The literal comparison returns false for identical literals because the datatype comparison returns false: https://github.com/apache/jena/blob/04f377d8aefc0cb8faeb62022d402ef17a6e6e96/jena-core/src/main/java/org/apache/jena/graph/impl/LiteralLabel.java#L477-L479
- `LiteralLabel` assumes that the datatypes can be compared by reference, which makes sense, because Jena maintains a global hashmap of all used datatypes in `TypeMapper`.
- From the debugger I got the info that somehow there were two instances of `BaseDatatype` with the same IRI on the heap (`http://www.opengis.net/ont/geosparql#wktLiteral`):
- Only one of these instances was registered in `TypeMapper.uriToDT`. I am constructing new datatypes only via the `NodeFactory`, which ALWAYS goes through the `TypeMapper`, so the other instance must have been overridden at some point. There is no other way it could have ended up on the heap.
I'm 99% sure that the issue is this part:
https://github.com/apache/jena/blob/04f377d8aefc0cb8faeb62022d402ef17a6e6e96/jena-core/src/main/java/org/apache/jena/datatypes/TypeMapper.java#L124-L140
If the datatype is not in the concurrent map, this is called:
https://github.com/apache/jena/blob/04f377d8aefc0cb8faeb62022d402ef17a6e6e96/jena-core/src/main/java/org/apache/jena/datatypes/TypeMapper.java#L187-L193
The call to `uriToDT.put()` will override the previous value in the map, if it exists. This is absolutely possible, because between the call to get() and put() we have no synchronization, so another thread may add the same datatype to the map in the meantime.
I don't have a reproducible example, but I think this is a pretty clear case of a race condition... also relatively easy to fix, I think. I'll try doing that.
### Relevant output and stacktrace
_No response_
### Are you interested in making a pull request?
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Ostrzyciel (via GitHub)" <gi...@apache.org>.
Ostrzyciel commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439658129
@arne-bdt What you propose is also a valid solution, but it would slow down datatyped literal comparisons somewhat.
I'm not sure what is the "grand vision" for how this should work in Jena, so I don't have a strong opinion here.
But, isn't it a separate issue? The problem of double-registration of datatypes is a bug one way or the other, right? My intuition is to first solve this bug, and then discuss how datatype equality should really work in Jena.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "arne-bdt (via GitHub)" <gi...@apache.org>.
arne-bdt closed issue #2795: Race condition in TypeMapper.getSafeTypeByName
URL: https://github.com/apache/jena/issues/2795
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Aklakan (via GitHub)" <gi...@apache.org>.
Aklakan commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439739754
> @arne-bdt on second thought and considering the point about breaking a contract, I think this would be a better solution [...]
I'd say computeIfAbsent is the right approach. The `uriToDT` and `classToDT` maps in TypeMapper are already ConcurrentHashMaps, so the issue with the double registration seems to be due to a race condition due to incorrect/sub-par use of the existing infrastructure, but there does not seem to be the need to require change to equals/hashCode of RDFDatatype. After all, the TypeMapper should ensure that later on reference equality works consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Ostrzyciel (via GitHub)" <gi...@apache.org>.
Ostrzyciel commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439448785
On a related note: calling this method can potentially wreak havoc: https://github.com/apache/jena/blob/04f377d8aefc0cb8faeb62022d402ef17a6e6e96/jena-core/src/main/java/org/apache/jena/datatypes/TypeMapper.java#L198C17-L198C35
I think that it should have (at least) a warning that calling it may break literal comparisons, if there are still old literal instances on the heap somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "arne-bdt (via GitHub)" <gi...@apache.org>.
arne-bdt commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439731093
> Isn't this a separate issue? The problem of duplicate registration of datatypes is a bug regardless, correct? My intuition is to address this bug first and then discuss how datatype equality should ideally function in Jena.
@Ostrzyciel `org.apache.jena.datatypes.TypeMapper#registerDatatype` behaves as documented: it registers the provided datatype and simply overrides any previously registered datatype with the same URI.
I really prefer your approach, where it's possible to check if the registration was successful or continue with the previously registered datatype, but that would break the existing contract.
This raises the question: Is it intentional for implementations of `RDFDatatype` not to override `#equals` and `#hashCode`? Should `RDFDatatypes` really be compared by reference only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Ostrzyciel (via GitHub)" <gi...@apache.org>.
Ostrzyciel commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439737229
@arne-bdt on second thought and considering the point about breaking a contract, I think this would be a better solution:
```java
public RDFDatatype getSafeTypeByName(final String uri) {
if (uri == null) {
// Plain literal
return null;
}
return uriToDT.computeIfAbsent(uri, uri -> {
// Unknown datatype
if (JenaParameters.enableSilentAcceptanceOfUnknownDatatypes) {
dtype = new BaseDatatype(uri);
// No need to update classToDT
// BaseDatatype.getJavaClass is always null
return registerDatatype(dtype);
} else {
throw new DatatypeFormatException(
"Attempted to created typed literal using an unknown datatype - " + uri);
}
});
}
```
And then leave `registerDatatype` unchanged. Note that I didn't test it... but it should work.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Ostrzyciel (via GitHub)" <gi...@apache.org>.
Ostrzyciel commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439732855
Hm, good point about breaking the contract. This could *possibly* break some applications which may register the same datatype more than once due to some library layering going on.
I'm out of my depth here. I think we need someone who knows the historical context and the intended contracts for these APIs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "Ostrzyciel (via GitHub)" <gi...@apache.org>.
Ostrzyciel commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439759834
I updated the PR #2796 with the computeIfAbsent approach. Comments welcome :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org
Re: [I] Race condition in TypeMapper.getSafeTypeByName [jena]
Posted by "arne-bdt (via GitHub)" <gi...@apache.org>.
arne-bdt commented on issue #2795:
URL: https://github.com/apache/jena/issues/2795#issuecomment-2439653100
> `LiteralLabel `assumes that the datatypes can be compared by reference.
LiteralLabel daoes not make such an assumption. `Objects.equals(a, b)` does not compare by reference. The code looks like this:
In my option the root cause of the whole problem is that the implementations of `RDFDatatype` do not override `equals(Object other)` and `hashCode()`.
So we indeed rely on reference equality, which I guess is a really bad thing.
That is what makes duplicated entries possible in the first place.
In the `testUserDefRepeated` from your [PR](https://github.com/apache/jena/pull/2796), I think the types should be equal:
```java
RDFDatatype type1 = new BaseDatatype("urn:x-jena-dt:repeated");
RDFDatatype type2 = new BaseDatatype("urn:x-jena-dt:repeated");
// Check precondition
assertEquals("Datatypes should be equal", type1, type2);
```
This assert currently fails.
I propose to implement `equals()` and `hashCode()` in `BaseDatatype` like this:
```java
@Override
public boolean equals(Object other) {
if(other != null
&& this.getURI().equals(((BaseDatatype)other).getURI())
&& this.getClass().equals(other.getClass())) {
return true;
}
return false;
}
@Override
public int hashCode() {
return super.uri.hashCode();
}
```
I am not sure if the implementing class should be part of the equality check. Checking it feels safer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@jena.apache.org
For additional commands, e-mail: issues-help@jena.apache.org