Fixed InputStreamContentProvider.hasNext() to be idempotent until next() is called.

jetty-client/src/main/java/org/eclipse/jetty/client/util/InputStreamContentProvider.java

@@ -25,6 +25,8 @@ import java.util.NoSuchElementException;
 
 import org.eclipse.jetty.client.api.ContentProvider;
 import org.eclipse.jetty.util.BufferUtil;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
 
 /**
  * A {@link ContentProvider} for an {@link InputStream}.
@@ -44,6 +46,8 @@ import org.eclipse.jetty.util.BufferUtil;
  */
 public class InputStreamContentProvider implements ContentProvider
 {
+    private static final Logger LOG = Log.getLogger(InputStreamContentProvider.class);
+
     private final InputStream stream;
     private final int bufferSize;
 
@@ -88,63 +92,91 @@ public class InputStreamContentProvider implements ContentProvider
     @Override
     public Iterator<ByteBuffer> iterator()
     {
-        return new Iterator<ByteBuffer>()
+        return new InputStreamIterator();
-        {
+    }
-            private final byte[] bytes = new byte[bufferSize];
-            private Exception failure;
-            private ByteBuffer buffer;
 
-            @Override
+    /**
-            public boolean hasNext()
+     * Iterating over an {@link InputStream} is tricky, because {@link #hasNext()} must return false
+     * if the stream reads -1. However, we don't know what to return until we read the stream, which
+     * means that stream reading must be performed by {@link #hasNext()}, which introduces a side-effect
+     * on what is supposed to be a simple query method (with respect to the Query Command Separation
+     * Principle).
+     * <p />
+     * Alternatively, we could return {@code true} from {@link #hasNext()} even if we don't know that
+     * we will read -1, but then when {@link #next()} reads -1 it must return an empty buffer.
+     * However this is problematic, since GETs with no content indication would become GET with chunked
+     * content, and not understood by servers.
+     * <p />
+     * Therefore we need to make sure that {@link #hasNext()} does not perform any side effect (so that
+     * it can be called multiple times) until {@link #next()} is called.
+     */
+    private class InputStreamIterator implements Iterator<ByteBuffer>
+    {
+        private final byte[] bytes = new byte[bufferSize];
+        private Exception failure;
+        private ByteBuffer buffer;
+        private Boolean hasNext;
+
+        @Override
+        public boolean hasNext()
+        {
+            try
             {
-                try
+                if (hasNext != null)
+                    return hasNext;
+
+                int read = stream.read(bytes);
+                LOG.debug("Read {} bytes from {}", read, stream);
+                if (read > 0)
                 {
-                    int read = stream.read(bytes);
+                    buffer = onRead(bytes, 0, read);
-                    if (read > 0)
+                    hasNext = Boolean.TRUE;
-                    {
+                    return true;
-                        buffer = onRead(bytes, 0, read);
-                        return true;
-                    }
-                    else if (read < 0)
-                    {
-                        return false;
-                    }
-                    else
-                    {
-                        buffer = BufferUtil.EMPTY_BUFFER;
-                        return true;
-                    }
                 }
-                catch (Exception x)
+                else if (read < 0)
                 {
-                    if (failure == null)
+                    hasNext = Boolean.FALSE;
-                    {
-                        failure = x;
-                        // Signal we have more content to cause a call to
-                        // next() which will throw NoSuchElementException.
-                        return true;
-                    }
                     return false;
                 }
+                else
+                {
+                    buffer = BufferUtil.EMPTY_BUFFER;
+                    hasNext = Boolean.TRUE;
+                    return true;
+                }
             }
-
+            catch (Exception x)
-            @Override
-            public ByteBuffer next()
             {
-                ByteBuffer result = buffer;
+                LOG.debug(x);
-                buffer = null;
+                if (failure == null)
-                if (failure != null)
+                {
-                    throw (NoSuchElementException)new NoSuchElementException().initCause(failure);
+                    failure = x;
-                if (result == null)
+                    // Signal we have more content to cause a call to
-                    throw new NoSuchElementException();
+                    // next() which will throw NoSuchElementException.
-                return result;
+                    hasNext = Boolean.TRUE;
+                    return true;
+                }
+                throw new IllegalStateException();
             }
+        }
 
-            @Override
+        @Override
-            public void remove()
+        public ByteBuffer next()
-            {
+        {
-                throw new UnsupportedOperationException();
+            if (failure != null)
-            }
+                throw (NoSuchElementException)new NoSuchElementException().initCause(failure);
-        };
+            ByteBuffer result = buffer;
+            if (result == null)
+                throw new NoSuchElementException();
+            buffer = null;
+            hasNext = null;
+            return result;
+        }
+
+        @Override
+        public void remove()
+        {
+            throw new UnsupportedOperationException();
+        }
     }
 }