Ensure attributes and contained object can be collected as fast as possible.

Motivation:

Due an implementation flaw in DefaultAttributeMap it was possible that an attribute and its stored value/key could not be collected until the DefaultAttributeMap could be collected. This can lead to unexpected memory usage and strong reachability of objects that should be collected.

Modifications:

Use an special empty DefaultAttribute as head of the each bucket which will not store any key / value. With this change everything can be collected as expected as we not use any DefaultAttribute created by the user as head of a bucket.

Result:

DefaultAttributeMap does not store user data and thus the lifetime of this user data is not tied to the lifetime of the DefaultAttributeMap.

common/src/main/java/io/netty/util/DefaultAttributeMap.java

@@ -68,11 +68,14 @@ public class DefaultAttributeMap implements AttributeMap {
         DefaultAttribute<?> head = attributes.get(i);
         if (head == null) {
             // No head exists yet which means we may be able to add the attribute without synchronization and just
-            // use compare and set. At worst we need to fallback to synchronization
+            // use compare and set. At worst we need to fallback to synchronization and waste two allocations.
-            head = new DefaultAttribute(key);
+            head = new DefaultAttribute();
+            DefaultAttribute<T> attr = new DefaultAttribute<T>(head, key);
+            head.next = attr;
+            attr.prev = head;
             if (attributes.compareAndSet(i, null, head)) {
-                // we were able to add it so return the head right away
+                // we were able to add it so return the attr right away
-                return (Attribute<T>) head;
+                return attr;
             } else {
                 head = attributes.get(i);
             }
@@ -81,19 +84,18 @@ public class DefaultAttributeMap implements AttributeMap {
         synchronized (head) {
             DefaultAttribute<?> curr = head;
             for (;;) {
-                if (!curr.removed && curr.key == key) {
-                    return (Attribute<T>) curr;
-                }
-
                 DefaultAttribute<?> next = curr.next;
                 if (next == null) {
                     DefaultAttribute<T> attr = new DefaultAttribute<T>(head, key);
-                    curr.next =  attr;
+                    curr.next = attr;
                     attr.prev = curr;
                     return attr;
-                } else {
-                    curr = next;
                 }
+
+                if (next.key == key && !next.removed) {
+                    return (Attribute<T>) next;
+                }
+                curr = next;
             }
         }
     }
@@ -116,16 +118,12 @@ public class DefaultAttributeMap implements AttributeMap {
             return false;
         }
 
-        // check on the head can be done without synchronization
+        // We need to synchronize on the head.
-        if (head.key == key && !head.removed) {
-            return true;
-        }
-
         synchronized (head) {
-            // we need to synchronize on the head
+            // Start with head.next as the head itself does not store an attribute.
             DefaultAttribute<?> curr = head.next;
             while (curr != null) {
-                if (!curr.removed && curr.key == key) {
+                if (curr.key == key && !curr.removed) {
                     return true;
                 }
                 curr = curr.next;
@@ -143,7 +141,7 @@ public class DefaultAttributeMap implements AttributeMap {
 
         private static final long serialVersionUID = -2661411462200283011L;
 
-        // The head of the linked-list this attribute belongs to, which may be itself
+        // The head of the linked-list this attribute belongs to
         private final DefaultAttribute<?> head;
         private final AttributeKey<T> key;
 
@@ -159,9 +157,10 @@ public class DefaultAttributeMap implements AttributeMap {
             this.key = key;
         }
 
-        DefaultAttribute(AttributeKey<T> key) {
+        // Special constructor for the head of the linked-list.
+        DefaultAttribute() {
             head = this;
-            this.key = key;
+            key = null;
         }
 
         @Override
@@ -197,23 +196,22 @@ public class DefaultAttributeMap implements AttributeMap {
 
         private void remove0() {
             synchronized (head) {
-                // We only update the linked-list structure if prev != null because if it is null this
+                assert head != null;
-                // DefaultAttribute acts also as head. The head must never be removed completely and just be
+                if (prev == null) {
-                // marked as removed as all synchronization is done on the head itself for each bucket.
+                    // Removed before.
-                // The head itself will be GC'ed once the DefaultAttributeMap is GC'ed. So at most 5 heads will
+                    return;
-                // be removed lazy as the array size is 5.
-                if (prev != null) {
-                    prev.next = next;
-
-                    if (next != null) {
-                        next.prev = prev;
-                    }
-
-                    // Null out prev and next - this will guard against multiple remove0() calls which may corrupt
-                    // the linked list for the bucket.
-                    prev = null;
-                    next = null;
                 }
+
+                prev.next = next;
+
+                if (next != null) {
+                    next.prev = prev;
+                }
+
+                // Null out prev and next - this will guard against multiple remove0() calls which may corrupt
+                // the linked list for the bucket.
+                prev = null;
+                next = null;
             }
         }
     }