groovy

A call to MetaClassRegistry.removeMetaClass() can unexpectedly cause other live entries to be removed

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Blocker Blocker
  • Resolution: Fixed
  • Affects Version/s: 1.6.3
  • Fix Version/s: 1.6.4, 1.7-beta-1
  • Component/s: None
  • Labels:
    None
  • Testcase included:
    yes
  • Patch Submitted:
    Yes
  • Number of attachments :
    1

Description

Due to a bug in the MemoryAwareConcurrentReadMap implementation, a call to remove entries from the MetaClassRegistry can result in other entries becoming inaccessible.

This can cause very unpredictable behavior/failures when relying on dynamic modifications to a Class' ExpandoMetaClass. For example, this bug is responsible for the behavior described in GRAILS-3666.

The failure occurs when a hash table entry that is not at the head of its bucket is removed. The mechanism to recreate the bucket's linked list after removal does not dereference the SoftReference objects for entries following the removal. As a result, any entries in the list after the removed entry will no longer be accessible by their original key.

A simple patch for the problem has been attached. Note that the fix is not necessary for the sremove() method, as proper dereferencing is used there.

The failure case can be easily demonstrated with the following test code:

import org.codehaus.groovy.runtime.metaclass.*;

def initalCapacity = 4
def loadFactor = 1000 // keep number of buckets low to increase entries per bucket
def keysToInsert = 2000

def map = new MemoryAwareConcurrentReadMap(initalCapacity, loadFactor)

def keys = []
keysToInsert.times
{
    def random = Math.random()
    map.put(random, random) // insert random key
    keys << random // keep track of keys so we can test reachability
}
println "Inserted $keysToInsert random keys"

int count = 0
keys.each
{
	if (map.get(it)) count++
}
println "Keys in map: $count"

def removeMe = map.get(keys[0]) // just remove any key
map.remove(removeMe)
println "Removed one key"

count = 0
keys.each
{
	if (map.get(it)) count++
}
println "Keys in map: $count"

assert count == keysToInsert - 1 //we expect our count to be decreased by one

In the example above, the load factor has been increased to an impracticable level to ensure failure will occur. The assertion will still fail with the default load factor, but not as consistently.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
blackdrag blackdrag added a comment -

this looks like a copy&paste error. Looking at the loop in the patch this class has 3 loops that look almost the same, but should do all the same.
expungeStallEntries is doing: head = new Entry(p.hash, p.key, p.value, head);
remove is doing: head = new Entry(p.hash, p.key, p.value, head, queue);
sremove is doing: head = new Entry(p.hash, p.getKey(), p.getValue(), head, queue);

I think they should all do the same, and I think they should all do it like expungeStallEntries, whcih avoids creating new SoftReferences for each key and value.

Show
blackdrag blackdrag added a comment - this looks like a copy&paste error. Looking at the loop in the patch this class has 3 loops that look almost the same, but should do all the same. expungeStallEntries is doing: head = new Entry(p.hash, p.key, p.value, head); remove is doing: head = new Entry(p.hash, p.key, p.value, head, queue); sremove is doing: head = new Entry(p.hash, p.getKey(), p.getValue(), head, queue); I think they should all do the same, and I think they should all do it like expungeStallEntries, whcih avoids creating new SoftReferences for each key and value.
Hide
Permalink
Stephen Mucher added a comment -

The Copy & Paste is from ConcurrentReaderHashMap, which didn't need to handle SoftReferences.

In MemoryAwareConcurrentReadMap, p.key() and p.value() are SoftRef objects, not Classes / MetaClasses. If you don't dereference them, you will end up with key -> SoftRef -> SoftRef -> Class. If you use Entry(int hash, SoftRef key, Reference value, Entry next) like expungeStallEntries(), the value property will be a dummy reference. (See constructor for Entry)

Thanks for the quick response, btw!

Show
Stephen Mucher added a comment - The Copy & Paste is from ConcurrentReaderHashMap , which didn't need to handle SoftReferences . In MemoryAwareConcurrentReadMap , p.key() and p.value() are SoftRef objects, not Classes / MetaClasses . If you don't dereference them, you will end up with key -> SoftRef -> SoftRef -> Class . If you use Entry(int hash, SoftRef key, Reference value, Entry next) like expungeStallEntries() , the value property will be a dummy reference. (See constructor for Entry ) Thanks for the quick response, btw!
Hide
Permalink
blackdrag blackdrag added a comment -

for reference:

        Entry(int hash, Object key, Object value, Entry next, ReferenceQueue queue) {
            this.hash = hash;
            this.key = new SoftRef(this,key,queue);
            this.next = next;
            this.value = new SoftRef(this,value,queue);
        }

        Entry(int hash, SoftRef key, Reference value, Entry next) {
            this.hash = hash;
            this.key = key;
            key.entry = this;
            this.next = next;
            this.value = DUMMY_REF;
            this.setValue(value);
        }

That means the version with the queue will create new references, while the version without the queue will not. But the seond constructor is not wrong, since this.key is a reference. So what expunge does is right and what sremove does is right, but what remove is doing is just plain wrong.

Show
blackdrag blackdrag added a comment - for reference: Entry( int hash, Object key, Object value, Entry next, ReferenceQueue queue) { this .hash = hash; this .key = new SoftRef( this ,key,queue); this .next = next; this .value = new SoftRef( this ,value,queue); } Entry( int hash, SoftRef key, Reference value, Entry next) { this .hash = hash; this .key = key; key.entry = this ; this .next = next; this .value = DUMMY_REF; this .setValue(value); } That means the version with the queue will create new references, while the version without the queue will not. But the seond constructor is not wrong, since this.key is a reference. So what expunge does is right and what sremove does is right, but what remove is doing is just plain wrong.
Hide
Permalink
Stephen Mucher added a comment -

Whoops – yeah, it looks like the second constructor should work too, and avoids needing to create new references. I got thrown off by the DUMMY_REF value and missed the this.setValue(value). I presume this.setValue(value) is being called instead of setting the value property directly to handle the null value case.

Show
Stephen Mucher added a comment - Whoops – yeah, it looks like the second constructor should work too, and avoids needing to create new references. I got thrown off by the DUMMY_REF value and missed the this.setValue(value) . I presume this.setValue(value) is being called instead of setting the value property directly to handle the null value case.
Hide
Permalink
blackdrag blackdrag added a comment -

fixed

Show
blackdrag blackdrag added a comment - fixed

People

  • Assignee:
    blackdrag blackdrag
    Reporter:
    Stephen Mucher
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: