From 3c11af965ad589f76c83e2a75f980483172596b7 Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Sat, 27 Apr 2019 11:32:50 -0700
Subject: [PATCH] DefaultHeaders#valueIterator doesn't remove from the in
 bucket list (#9090)

Motivation:
DefaultHeaders entries maintains two linked lists. 1 for overall insertion order
and 1 for "in bucket" order. DefaultHeaders#valueIterator removal (introduced in 1d9090aab231ab737bd6459e0369b30d752296b2) only reliably
removes the entry from the overall insertion order, but may not remove from the
bucket unless the element is the first entry.

Modifications:
- DefaultHeaders$ValueIterator should track 2 elements behind the next entry so
that the single linked "in bucket" list can be patched up when removing the
previous entry.

Result:
More correct DefaultHeaders#valueIterator removal.
---
 .../netty/handler/codec/DefaultHeaders.java   | 12 +++++-
 .../handler/codec/DefaultHeadersTest.java     | 42 +++++++++++++++----
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
index b3d1c58fee..aa69a0423b 100644
--- a/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
+++ b/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java
@@ -1007,14 +1007,18 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
         return value;
     }
 
-    private void remove0(HeaderEntry<K, V> entry) {
+    private HeaderEntry<K, V> remove0(HeaderEntry<K, V> entry, HeaderEntry<K, V> previous) {
         int i = index(entry.hash);
         HeaderEntry<K, V> e = entries[i];
         if (e == entry) {
             entries[i] = entry.next;
+            previous = entries[i];
+        } else {
+            previous.next = entry.next;
         }
         entry.remove();
         --size;
+        return previous;
     }
 
     @SuppressWarnings("unchecked")
@@ -1060,6 +1064,7 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
     private final class ValueIterator implements Iterator<V> {
         private final K name;
         private final int hash;
+        private HeaderEntry<K, V> removalPrevious;
         private HeaderEntry<K, V> previous;
         private HeaderEntry<K, V> next;
 
@@ -1079,6 +1084,9 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
             if (!hasNext()) {
                 throw new NoSuchElementException();
             }
+            if (previous != null) {
+                removalPrevious = previous;
+            }
             previous = next;
             calculateNext(next.next);
             return previous.value;
@@ -1089,7 +1097,7 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
             if (previous == null) {
                 throw new IllegalStateException();
             }
-            remove0(previous);
+            removalPrevious = remove0(previous, removalPrevious);
             previous = null;
         }
 
diff --git a/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java b/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java
index c6aaa3d6d8..3bacfaf8f8 100644
--- a/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java
+++ b/codec/src/test/java/io/netty/handler/codec/DefaultHeadersTest.java
@@ -110,24 +110,50 @@ public class DefaultHeadersTest {
     }
 
     @Test
-    public void multipleValuesPerNameIterator() {
+    public void multipleValuesPerNameIteratorWithOtherNames() {
         TestDefaultHeaders headers = newInstance();
-        headers.add(of("name"), of("value1"));
-        headers.add(of("name"), of("value2"));
-        headers.add(of("name"), of("value3"));
-        assertEquals(3, headers.size());
+        headers.add(of("name1"), of("value1"));
+        headers.add(of("name1"), of("value2"));
+        headers.add(of("name2"), of("value4"));
+        headers.add(of("name1"), of("value3"));
+        assertEquals(4, headers.size());
 
         List<CharSequence> values = new ArrayList<>();
-        Iterator<CharSequence> itr = headers.valueIterator(of("name"));
+        Iterator<CharSequence> itr = headers.valueIterator(of("name1"));
         while (itr.hasNext()) {
             values.add(itr.next());
             itr.remove();
         }
         assertEquals(3, values.size());
+        assertEquals(1, headers.size());
+        assertFalse(headers.isEmpty());
+        assertTrue(values.containsAll(asList(of("value1"), of("value2"), of("value3"))));
+        itr = headers.valueIterator(of("name1"));
+        assertFalse(itr.hasNext());
+        itr = headers.valueIterator(of("name2"));
+        assertTrue(itr.hasNext());
+        assertEquals(of("value4"), itr.next());
+        assertFalse(itr.hasNext());
+    }
+
+    @Test
+    public void multipleValuesPerNameIterator() {
+        TestDefaultHeaders headers = newInstance();
+        headers.add(of("name1"), of("value1"));
+        headers.add(of("name1"), of("value2"));
+        assertEquals(2, headers.size());
+
+        List<CharSequence> values = new ArrayList<CharSequence>();
+        Iterator<CharSequence> itr = headers.valueIterator(of("name1"));
+        while (itr.hasNext()) {
+            values.add(itr.next());
+            itr.remove();
+        }
+        assertEquals(2, values.size());
         assertEquals(0, headers.size());
         assertTrue(headers.isEmpty());
-        assertTrue(values.containsAll(asList(of("value1"), of("value2"), of("value3"))));
-        itr = headers.valueIterator(of("name"));
+        assertTrue(values.containsAll(asList(of("value1"), of("value2"))));
+        itr = headers.valueIterator(of("name1"));
         assertFalse(itr.hasNext());
     }