[jira] [Comment Edited] (SLING-2786) ResourceMetadata contains stale unmodifiableMap cache after clone()

["Lukas Eder (JIRA)" <ji...@apache.org>]

    [ 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