BasicPersistentEntity.getPersistentProperty(…) returns the unchecked value in ConcurrentReferenceHashMap [DATACMNS-1364]

[spring-projects-issues]

Toshiaki Maki opened DATACMNS-1364 and commented

ConcurrentReferenceHashMap was introduced by this commit. However getPersistentProperty still uses propertyCache.get(name).

https://github.com/spring-projects/spring-data-commons/blob/2.1.0.RC1/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java#L309

Javadoc says

NOTE: The use of references means that there is no guarantee that items placed into the map will be subsequently available. The garbage collector may discard references at any time, so it may appear that an unknown thread is silently removing entries. 

I had some issues  related to this after several hours after deploying an application.
here is an example.

Using computeIfAbsent or ConcurrentHashMap will solve this issue.
 

---

Affects: 2.1 RC1 (Lovelace), 2.0.9 (Kay SR9)

Issue Links:

• DATAMONGO-2042 Longevity issue results in error "No property 'id' found on class ...."
  ("duplicates")

• DATACMNS-1393 Floating deadock for BasicPersistentEntity getPersistentProperty method
  ("is duplicated by")

• DATACMNS-1210 Thread-safetey issue in annotation detection in BasicPersistentEntity

• DATAMONGO-2038 InvalidPersistentPropertyPath after updating Spring data version

Referenced from: pull request #304

Backported to: 2.0.10 (Kay SR10)

0 votes, 5 watchers

[spring-projects-issues]

Mark Paluch commented

Thanks a lot for your analysis. We've seen a couple of other tickets popping up related to the issue.

The actual problem is that we've changed multiple maps to ConcurrentReferenceHashMap. The property cache isn't an actual cache but holds the properties without a possibility to recalculate/recreate a property. GC activity causes eviction and so properties get lost.

The correct implementation is to use a ConcurrentHashMap as properties depend on their PersistentEntity

[spring-projects-issues]

Mark Paluch commented

After another pass it turns out that we do not require a Map that allows concurrent modifications as entities are created single-threaded. So a HashMap is sufficient and concurrent reads do not modify the data structure. HashMap also allows null keys and values whereas ConcurrentHashMap does not