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 1d9090aab2) 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.
This commit is contained in:
Scott Mitchell 2019-04-27 11:32:50 -07:00 committed by GitHub
parent 2c12f09ec9
commit 2d33d1493e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 44 additions and 10 deletions

View File

@ -1012,14 +1012,18 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
return value; 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); int i = index(entry.hash);
HeaderEntry<K, V> e = entries[i]; HeaderEntry<K, V> e = entries[i];
if (e == entry) { if (e == entry) {
entries[i] = entry.next; entries[i] = entry.next;
previous = entries[i];
} else {
previous.next = entry.next;
} }
entry.remove(); entry.remove();
--size; --size;
return previous;
} }
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
@ -1065,6 +1069,7 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
private final class ValueIterator implements Iterator<V> { private final class ValueIterator implements Iterator<V> {
private final K name; private final K name;
private final int hash; private final int hash;
private HeaderEntry<K, V> removalPrevious;
private HeaderEntry<K, V> previous; private HeaderEntry<K, V> previous;
private HeaderEntry<K, V> next; private HeaderEntry<K, V> next;
@ -1084,6 +1089,9 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
if (!hasNext()) { if (!hasNext()) {
throw new NoSuchElementException(); throw new NoSuchElementException();
} }
if (previous != null) {
removalPrevious = previous;
}
previous = next; previous = next;
calculateNext(next.next); calculateNext(next.next);
return previous.value; return previous.value;
@ -1094,7 +1102,7 @@ public class DefaultHeaders<K, V, T extends Headers<K, V, T>> implements Headers
if (previous == null) { if (previous == null) {
throw new IllegalStateException(); throw new IllegalStateException();
} }
remove0(previous); removalPrevious = remove0(previous, removalPrevious);
previous = null; previous = null;
} }

View File

@ -110,24 +110,50 @@ public class DefaultHeadersTest {
} }
@Test @Test
public void multipleValuesPerNameIterator() { public void multipleValuesPerNameIteratorWithOtherNames() {
TestDefaultHeaders headers = newInstance(); TestDefaultHeaders headers = newInstance();
headers.add(of("name"), of("value1")); headers.add(of("name1"), of("value1"));
headers.add(of("name"), of("value2")); headers.add(of("name1"), of("value2"));
headers.add(of("name"), of("value3")); headers.add(of("name2"), of("value4"));
assertEquals(3, headers.size()); headers.add(of("name1"), of("value3"));
assertEquals(4, headers.size());
List<CharSequence> values = new ArrayList<CharSequence>(); List<CharSequence> values = new ArrayList<CharSequence>();
Iterator<CharSequence> itr = headers.valueIterator(of("name")); Iterator<CharSequence> itr = headers.valueIterator(of("name1"));
while (itr.hasNext()) { while (itr.hasNext()) {
values.add(itr.next()); values.add(itr.next());
itr.remove(); itr.remove();
} }
assertEquals(3, values.size()); 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()); assertEquals(0, headers.size());
assertTrue(headers.isEmpty()); assertTrue(headers.isEmpty());
assertTrue(values.containsAll(asList(of("value1"), of("value2"), of("value3")))); assertTrue(values.containsAll(asList(of("value1"), of("value2"))));
itr = headers.valueIterator(of("name")); itr = headers.valueIterator(of("name1"));
assertFalse(itr.hasNext()); assertFalse(itr.hasNext());
} }