[
https://issues.apache.org/jira/browse/SLING-2786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13600963#comment-13600963
]
Lukas Eder commented on SLING-2786:
-----------------------------------
Thanks for the feedback. I was just going to implement these modifications:
- Override clone(), resetting the three new cache objects introduced in
SLING-2785: lockedEntrySet, lockedKeySet, lockedValues
- Marking them all as transient
- Marking lockedEntrySet as volatile to enforce correct behaviour of
double-checked locking in getLockedData()
But then, I've noticed that the current implementation might still be wrong.
Consider this piece of code:
final ResourceMetadata m = new ResourceMetadata();
m.put("key", "value");
Set<String> keys = m.keySet();
m.lock();
keys.remove("key1");
This implies that ResourceMetadata should probably return inner class
implementations for keySet(), entrySet(), and values(), referencing the outer
instance and its isReadOnly flag. Attached is a patch with more (currently
failing) test cases
> ResourceMetadata contains stale unmodifiableMap cache after clone()
> -------------------------------------------------------------------
>
> Key: SLING-2786
> URL: https://issues.apache.org/jira/browse/SLING-2786
> Project: Sling
> Issue Type: Bug
> Components: API
> Affects Versions: API 2.3.0
> Reporter: Lukas Eder
> Priority: Minor
>
> Super types of ResourceMetadata (java.util.HashMap and java.util.AbstractMap)
> correctly implement caching for members such as entrySet, keySet, values,
> etc. Two things should be considered:
> 1. The cache should be marked transient. It is not desireable to serialise /
> deserialise the "unmodifiableMap" cache. Only the isReadOnly flag should be
> serialised
> 2. clone() should be overridden in order to reset the cache on cloned
> instances
> Consider the following code:
> ResourceMetadata map1 = new ResourceMetadata();
> map1.put("key1", "value1");
> map1.lock();
> map1.values(); // Enforce the creation of the cache
> ResourceMetadata map2 = (ResourceMetadata) map1.clone();
> Now, map2 contains an unmodifiable wrapper of map1. While this generally
> behaves correctly, there are two problems:
> a) This is a potential memory leak as clones hold a reference to the original
> map.
> b) If unlock() is ever added, this might lead to a subtle bug when (after the
> above code):
> - map1.unlock() is called
> - map1 is modified
> - map2 will reflect map1's modifications, even if map2 should still be
> locked
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
