Header iterator memory optimization in HeaderGroup

httpcore5/src/main/java/org/apache/hc/core5/http/impl/DefaultContentLengthStrategy.java

@@ -27,6 +27,7 @@
 
 package org.apache.hc.core5.http.impl;
 
+import java.util.Iterator;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.hc.core5.annotation.Contract;
@@ -70,24 +71,26 @@ enum Coding { UNKNOWN, CHUNK }
     @Override
     public long determineLength(final HttpMessage message) throws HttpException {
         Args.notNull(message, "HTTP message");
-        final Header teh = message.getFirstHeader(HttpHeaders.TRANSFER_ENCODING);
-        if (teh != null) {
+        final Iterator<Header> it = message.headerIterator(HttpHeaders.TRANSFER_ENCODING);
+        if (it.hasNext()) {
             final AtomicReference<Coding> codingRef = new AtomicReference<>();
-            MessageSupport.parseTokens(message, HttpHeaders.TRANSFER_ENCODING, e -> {
-                if (!TextUtils.isBlank(e)) {
-                    if (e.equalsIgnoreCase(HeaderElements.CHUNKED_ENCODING)) {
-                        if (!codingRef.compareAndSet(null, Coding.CHUNK)) {
+            while (it.hasNext()) {
+                MessageSupport.parseTokens(it.next(), e -> {
+                    if (!TextUtils.isBlank(e)) {
+                        if (e.equalsIgnoreCase(HeaderElements.CHUNKED_ENCODING)) {
+                            if (!codingRef.compareAndSet(null, Coding.CHUNK)) {
+                                codingRef.set(Coding.UNKNOWN);
+                            }
+                        } else {
                             codingRef.set(Coding.UNKNOWN);
                         }
-                    } else {
-                        codingRef.set(Coding.UNKNOWN);
                     }
-                }
-            });
+                });
+            }
             if (codingRef.get() == Coding.CHUNK) {
                 return CHUNKED;
             }
-            throw new NotImplementedException("Unsupported transfer encoding: " + teh.getValue());
+            throw new NotImplementedException("Unsupported transfer encoding");
         }
         final long contentLength = MessageSupport.getContentLength(message);
         return contentLength >= 0 ? contentLength : UNDEFINED;

httpcore5/src/main/java/org/apache/hc/core5/http/message/BasicListHeaderIterator.java

@@ -81,6 +81,14 @@ public BasicListHeaderIterator(final List<? extends Header> headers, final Strin
         this.lastIndex = -1;
     }
 
+    BasicListHeaderIterator(final List<? extends Header> headers, final int currentIndex, final String name) {
+        super();
+        this.allHeaders = headers;
+        this.headerName = name;
+        this.currentIndex = currentIndex;
+        this.lastIndex = -1;
+    }
+
     /**
      * Determines the index of the next header.
      *

httpcore5/src/main/java/org/apache/hc/core5/http/message/HeaderGroup.java

@@ -345,10 +345,19 @@ public int countHeaders(final String name) {
      *
      * @return iterator over this group of headers.
      *
+     * <p>
+     * IMPORTANT: please note that if the header group mutates while the
+     * iterator returned by this method still has pending elements
+     * the sequence of headers produced by such iterator is considered
+     * unstable and can be incorrect.
+     *
      * @since 5.0
      */
     @Override
     public Iterator<Header> headerIterator() {
+        if (this.headers.isEmpty()) {
+            return NullHeaderIterator.INSTANCE;
+        }
         return new BasicListHeaderIterator(this.headers, null);
     }
 
@@ -360,10 +369,28 @@ public Iterator<Header> headerIterator() {
      *
      * @return iterator over some headers in this group.
      *
+     * <p>
+     * IMPORTANT: please note that if the header group mutates while the
+     * iterator returned by this method still has pending elements
+     * the sequence of headers produced by such iterator is considered
+     * unstable and can be incorrect.
+     *
      * @since 5.0
      */
     @Override
     public Iterator<Header> headerIterator(final String name) {
+        if (this.headers.isEmpty()) {
+            return NullHeaderIterator.INSTANCE;
+        }
+        if (name != null) {
+            for (int i = 0; i < this.headers.size(); i++) {
+                final Header h = this.headers.get(i);
+                if (h.getName().equalsIgnoreCase(name)) {
+                    return new BasicListHeaderIterator(this.headers, i, name);
+                }
+            }
+            return NullHeaderIterator.INSTANCE;
+        }
         return new BasicListHeaderIterator(this.headers, name);
     }
 

httpcore5/src/main/java/org/apache/hc/core5/http/message/MessageSupport.java

@@ -53,6 +53,7 @@
 import org.apache.hc.core5.http.MessageHeaders;
 import org.apache.hc.core5.http.Method;
 import org.apache.hc.core5.http.NameValuePair;
+import org.apache.hc.core5.http.ParseException;
 import org.apache.hc.core5.http.ProtocolException;
 import org.apache.hc.core5.util.Args;
 import org.apache.hc.core5.util.CharArrayBuffer;
@@ -65,6 +66,8 @@
  */
 public class MessageSupport {
 
+    private static final Tokenizer TK = Tokenizer.INSTANCE;
+
     private MessageSupport() {
         // Do not allow utility class to be instantiated.
     }
@@ -244,6 +247,7 @@ public static void parseElementList(final CharSequence src,
         Args.notNull(consumer, "Consumer");
         while (!cursor.atEnd()) {
             consumer.accept(src, cursor);
+            TK.skipWhiteSpace(src, cursor);
             if (!cursor.atEnd()) {
                 final char ch = src.charAt(cursor.getPos());
                 if (ch == ',') {
@@ -306,10 +310,14 @@ public static void parseElementListStrict(final CharSequence src,
         Args.notNull(consumer, "Consumer");
         while (!cursor.atEnd()) {
             consumer.accept(src, cursor);
+            TK.skipWhiteSpace(src, cursor);
             if (!cursor.atEnd()) {
                 final char ch = src.charAt(cursor.getPos());
                 if (ch == ',') {
                     cursor.updatePos(cursor.getPos() + 1);
+                } else {
+                    throw new ParseException("Invalid header element",
+                            src, cursor.getLowerBound(), cursor.getUpperBound(), cursor.getPos());
                 }
             }
         }
@@ -332,10 +340,16 @@ public static void parseTokens(final CharSequence src,
                                    final ParserCursor cursor,
                                    final Tokenizer.Delimiter delimiterPredicate,
                                    final Consumer<String> consumer) {
-        parseElementList(src, cursor, (sequence, c) -> {
-            final String token = Tokenizer.INSTANCE.parseToken(src, c, delimiterPredicate);
+        while (!cursor.atEnd()) {
+            final String token = Tokenizer.INSTANCE.parseToken(src, cursor, delimiterPredicate);
             consumer.accept(token);
-        });
+            if (!cursor.atEnd()) {
+                final char ch = src.charAt(cursor.getPos());
+                if (delimiterPredicate != null && delimiterPredicate.test(ch)) {
+                    cursor.updatePos(cursor.getPos() + 1);
+                }
+            }
+        }
     }
 
     /**

httpcore5/src/main/java/org/apache/hc/core5/http/message/NullHeaderIterator.java

@@ -0,0 +1,54 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation.  For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.message;
+
+import java.util.Iterator;
+import java.util.NoSuchElementException;
+
+import org.apache.hc.core5.http.Header;
+
+final class NullHeaderIterator implements Iterator<Header> {
+
+    final static NullHeaderIterator INSTANCE = new NullHeaderIterator();
+
+    @Override
+    public boolean hasNext() {
+        return false;
+    }
+
+    @Override
+    public Header next() throws NoSuchElementException {
+        throw new NoSuchElementException("Iteration already finished.");
+    }
+
+    @Override
+    public void remove() throws UnsupportedOperationException {
+        throw new UnsupportedOperationException("Removing headers is not supported.");
+    }
+
+}

httpcore5/src/test/java/org/apache/hc/core5/http/message/TestHeaderGroup.java

@@ -162,13 +162,65 @@ void testCondensedHeader() {
     }
 
     @Test
-    void testIterator() {
+    void testEmptyListIterator() {
         final HeaderGroup headergroup = new HeaderGroup();
         final Iterator<Header> i = headergroup.headerIterator();
         Assertions.assertNotNull(i);
         Assertions.assertFalse(i.hasNext());
     }
 
+    @Test
+    void testEmptyListIteratorByName() {
+        final HeaderGroup headergroup = new HeaderGroup();
+        final Iterator<Header> i = headergroup.headerIterator("some-header");
+        Assertions.assertNotNull(i);
+        Assertions.assertFalse(i.hasNext());
+    }
+
+    @Test
+    void testNonEmptyListIteratorByName() {
+        final HeaderGroup headergroup = new HeaderGroup();
+        headergroup.setHeaders(
+                new BasicHeader("a", "a-one"),
+                new BasicHeader("b", "b-one"),
+                new BasicHeader("a", "a-two"),
+                new BasicHeader("b", "b-two"),
+                new BasicHeader("b", "b-three"));
+        final Iterator<Header> it1 = headergroup.headerIterator("a");
+        Assertions.assertNotNull(it1);
+        Assertions.assertTrue(it1.hasNext());
+        Assertions.assertEquals("a-one", it1.next().getValue());
+        Assertions.assertTrue(it1.hasNext());
+        Assertions.assertEquals("a-two", it1.next().getValue());
+        Assertions.assertFalse(it1.hasNext());
+
+        final Iterator<Header> it2 = headergroup.headerIterator("b");
+        Assertions.assertNotNull(it2);
+        Assertions.assertTrue(it2.hasNext());
+        Assertions.assertEquals("b-one", it2.next().getValue());
+        Assertions.assertTrue(it2.hasNext());
+        Assertions.assertEquals("b-two", it2.next().getValue());
+        Assertions.assertTrue(it2.hasNext());
+        Assertions.assertEquals("b-three", it2.next().getValue());
+        Assertions.assertFalse(it2.hasNext());
+    }
+
+
+    @Test
+    void testIteratorByNameRemoveBeforeNext() {
+        final HeaderGroup headerGroup = new HeaderGroup();
+        final Header headerA = new BasicHeader("a", "a-one");
+        final Header headerB = new BasicHeader("b", "b-one");
+        headerGroup.setHeaders(headerA, headerB);
+
+        final Iterator<Header> iterator = headerGroup.headerIterator("b");
+
+        Assertions.assertThrows(IllegalStateException.class, iterator::remove);
+        Assertions.assertArrayEquals(
+                new Header[] { headerA, headerB },
+                headerGroup.getHeaders());
+    }
+
     @Test
     void testHeaderRemove() {
         final HeaderGroup headergroup = new HeaderGroup();

httpcore5/src/test/java/org/apache/hc/core5/http/message/TestMessageSupport.java

@@ -45,12 +45,15 @@
 import org.apache.hc.core5.http.HttpMessage;
 import org.apache.hc.core5.http.HttpResponse;
 import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.HttpVersion;
 import org.apache.hc.core5.http.Method;
 import org.apache.hc.core5.http.NameValuePair;
 import org.apache.hc.core5.http.ProtocolException;
+import org.apache.hc.core5.http.ProtocolVersion;
 import org.apache.hc.core5.http.io.entity.HttpEntities;
 import org.apache.hc.core5.http.support.BasicResponseBuilder;
 import org.apache.hc.core5.util.CharArrayBuffer;
+import org.apache.hc.core5.util.Tokenizer;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
@@ -110,6 +113,15 @@ void testParseTokensWithConsumer() {
         Assertions.assertEquals(Arrays.asList("a", "b", "c", "c"), tokens);
     }
 
+    @Test
+    void testParseTokensWithConsumerAndCustomDelimiter() {
+        final String s = "a, b ;  c, c";
+        final ParserCursor cursor = new ParserCursor(0, s.length());
+        final List<String> tokens = new ArrayList<>();
+        MessageSupport.parseTokens(s, cursor, Tokenizer.delimiters(';'), tokens::add);
+        Assertions.assertEquals(Arrays.asList("a, b", "c, c"), tokens);
+    }
+
     @Test
     void testParseTokenHeaderWithConsumer() {
         final Header header = new BasicHeader(HttpHeaders.TRAILER, "a, b, c, c");
@@ -382,6 +394,27 @@ void testParseHeaders() {
         Assertions.assertEquals(Arrays.asList("this", "that", "this", "that", "what not"), tokens);
     }
 
+    @Test
+    void testParseHeadersElementWhitespace() throws Exception {
+        final HttpMessage message = new BasicHttpRequest(Method.GET, "/");
+        message.addHeader("Some-Header", "HTTP/1.0");
+        message.addHeader("Some-Header", "   HTTP/1.1  ");
+        message.addHeader("Some-Header", " HTTP/2   , HTTP/2.0   , HTTP/0.9  ");
+
+        final List<String> versions = new LinkedList<>();
+        MessageSupport.parseElementList(message, "Some-header", (charSequence, cursor) -> {
+            final String ver = copyToken(charSequence, cursor);
+            versions.add(ver);
+        });
+        Assertions.assertEquals(Arrays.asList(
+                        "HTTP/1.0",
+                        "HTTP/1.1",
+                        "HTTP/2",
+                        "HTTP/2.0",
+                        "HTTP/0.9"),
+                versions);
+    }
+
     @Test
     void testParseHeadersStrict() throws Exception {
         final HttpMessage message = new BasicHttpRequest(Method.GET, "/");
@@ -415,6 +448,41 @@ void testParseHeadersStrict() throws Exception {
                 }));
     }
 
+    @Test
+    void testParseHeadersStrictElementWhitespace() throws Exception {
+        final HttpMessage message = new BasicHttpRequest(Method.GET, "/");
+        message.addHeader("Some-Header", "HTTP/1.0");
+        message.addHeader("Some-Header", "   HTTP/1.1  ");
+        message.addHeader("Some-Header", " HTTP/2   , HTTP/2.0   , HTTP/0.9  ");
+
+        final List<ProtocolVersion> versions = new LinkedList<>();
+        MessageSupport.parseElementListStrict(message, "Some-header", (charSequence, cursor) -> {
+            final ProtocolVersion ver = HttpVersion.parse(charSequence, cursor, Tokenizer.delimiters(','));
+            versions.add(ver);
+        });
+        Assertions.assertEquals(Arrays.asList(
+                HttpVersion.HTTP_1_0,
+                HttpVersion.HTTP_1_1,
+                HttpVersion.HTTP_2_0,
+                HttpVersion.HTTP_2_0,
+                HttpVersion.HTTP_0_9),
+                versions);
+    }
+
+    @Test
+    void testParseHeadersStrictInvalidElement() throws Exception {
+        final HttpMessage message = new BasicHttpRequest(Method.GET, "/");
+        message.addHeader("Some-Header", "HTTP/1.0");
+        message.addHeader("Some-Header", "   HTTP/1.1  HTTP/1.1");
+
+        Assertions.assertThrows(ProtocolException.class, () -> {
+                    MessageSupport.parseElementListStrict(message, "Some-header", (charSequence, cursor) -> {
+                        HttpVersion.parse(charSequence, cursor, Tokenizer.delimiters(','));
+                    });
+                }
+        );
+    }
+
     @Test
     void testAddContentHeaders() {
         final HttpEntity entity = HttpEntities.create("some stuff with trailers", StandardCharsets.US_ASCII,