[
https://issues.apache.org/jira/browse/SLING-2786?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13600963#comment-13600963
]
Lukas Eder edited comment on SLING-2786 at 3/13/13 9:03 AM:
------------------------------------------------------------
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 shouldn't work anymore, as the view's
referenced map has been locked
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
was (Author: lukas.eder):
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
> Attachments: ResourceMetadataTest.java.patch
>
>
> 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
