[Cronet] Fix CronetFixedModeOutputStream to not write more bytes than specified

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net.urlconnection;
 
 import org.chromium.base.VisibleForTesting;
 import org.chromium.net.UploadDataProvider;
 import org.chromium.net.UploadDataSink;
 
@@ skipping 12 matching lines @@
 final class CronetFixedModeOutputStream extends CronetOutputStream {
     // CronetFixedModeOutputStream buffers up to this value and wait for UploadDataStream
     // to consume the data. This field is non-final, so it can be changed for tests.
     // Using 16384 bytes is because the internal read buffer is 14520 for QUIC,
     // 16384 for SPDY, and 16384 for normal HTTP/1.1 stream.
     @VisibleForTesting
     private static int sDefaultBufferLength = 16384;
     private final CronetHttpURLConnection mConnection;
     private final MessageLoop mMessageLoop;
     private final long mContentLength;
+    // Internal buffer for storing the bytes written by consumer and for providing

[pauljensen, 2016/06/10 17:45:44]

"written" has several possible meanings here (e.g. writing to the server,
writing to mBuffer, writing to the UploadDataSink), as does consumer (e.g.
Cronet API consumer,  consumer in producer consumer model), how about "Internal
buffer used to hold the bytes from the client"?

[xunjieli, 2016/06/10 18:48:12]

Done.

+    // bytes for the native stack to consume in UploadDataProvider.read().

[pauljensen, 2016/06/10 17:45:44]

the consuming isn't really done in UploadDataProvider.read(), how about "...from
the client until they are copied to the UploadDataSink in
UploadDataProvider.read()"

[xunjieli, 2016/06/10 18:48:12]

Done.

+    // CronetFixedModeOutputStream allows consumer to write up to sDefaultBufferLength,

[pauljensen, 2016/06/10 17:45:44]

consumer->the client

[pauljensen, 2016/06/10 17:45:45]

sDefaultBufferLength->sDefaultBufferLength bytes

[pauljensen, 2016/06/10 17:45:45]

write->provide

[xunjieli, 2016/06/10 18:48:12]

Done.

[xunjieli, 2016/06/10 18:48:12]

Done.

[xunjieli, 2016/06/10 18:48:13]

Done.

+    // and wait for UploadDataProvider.read() to be called so mBuffer is cleared

[pauljensen, 2016/06/10 17:45:44]

so->, after which point

[xunjieli, 2016/06/10 18:48:11]

Done.

+    // for next round of writing. During writing, the buffer's position points

[pauljensen, 2016/06/10 17:45:44]

for next round of writing->so the client can fill it again.

[pauljensen, 2016/06/10 17:45:44]

During writing->While the client is filling the buffer (via {@code write()})

[xunjieli, 2016/06/10 18:48:11]

Done.

[xunjieli, 2016/06/10 18:48:12]

Done.

+    // to the next byte to be written, and limit is the index at which no byte

[pauljensen, 2016/06/10 17:45:44]

"is the index..."->"points to the end of the buffer"

[pauljensen, 2016/06/10 17:45:44]

written->provided by the client

[xunjieli, 2016/06/10 18:48:12]

Done.

+    // should be written. The buffer is flipped before it is passed to the
+    // UploadDataProvider for consuming. Once it is flipped, buffer position
+    // points to the next byte to be read, and limit is the index at which no

[pauljensen, 2016/06/10 17:45:44]

read->copied to the UploadDataSink

[pauljensen, 2016/06/10 17:45:44]

"is the index..."->"points to the end of the data available to be copied to the
UploadDataSink"

[xunjieli, 2016/06/10 18:48:11]

Done.

[xunjieli, 2016/06/10 18:48:12]

Done.

+    // byte should be read. When the UploadDataProvider is done reading from

[pauljensen, 2016/06/10 17:45:44]

"is done..."->"has provided all remaining bytes from the buffer to the
UploadDataSink, it clears the buffer so the client can fill it again"

[xunjieli, 2016/06/10 18:48:11]

Done.

+    // mBuffer, it clears mBuffer for next round of writing.
     private final ByteBuffer mBuffer;
     private final UploadDataProvider mUploadDataProvider = new UploadDataProviderImpl();
     private long mBytesWritten;
 
     /**
      * Package protected constructor.
      * @param connection The CronetHttpURLConnection object.
      * @param contentLength The content length of the request body. Non-zero for
      *            non-chunked upload.
      */
@@ skipping 10 matching lines @@
         int bufferSize = (int) Math.min(mContentLength, sDefaultBufferLength);
         mBuffer = ByteBuffer.allocate(bufferSize);
         mConnection = connection;
         mMessageLoop = messageLoop;
         mBytesWritten = 0;
     }
 
     @Override
     public void write(int oneByte) throws IOException {
         checkNotExceedContentLength(1);
-        while (mBuffer.position() == mBuffer.limit()) {
-            // Wait until buffer is consumed.
-            mMessageLoop.loop();
-        }
+        ensureBufferHasRemaining();
         mBuffer.put((byte) oneByte);
         mBytesWritten++;
-        if (mBytesWritten == mContentLength) {
-            // Entire post data has been received. Now wait for network stack to
-            // read it.
-            mMessageLoop.loop();
-        }
+        uploadIfComplete();
     }
 
     @Override
     public void write(byte[] buffer, int offset, int count) throws IOException {
         if (buffer.length - offset < count || offset < 0 || count < 0) {
             throw new IndexOutOfBoundsException();
         }
         checkNotExceedContentLength(count);
-        if (count == 0) {
-            return;
-        }
         int toSend = count;
         while (toSend > 0) {
-            if (mBuffer.position() == mBuffer.limit()) {
-                // Wait until buffer is consumed.
-                mMessageLoop.loop();
-            }
-            int sent = Math.min(toSend, mBuffer.limit() - mBuffer.position());
+            ensureBufferHasRemaining();
+            int sent = Math.min(toSend, mBuffer.remaining());
             mBuffer.put(buffer, offset + count - toSend, sent);
             toSend -= sent;
         }
         mBytesWritten += count;
+        uploadIfComplete();
+    }
+
+    /**
+     * If {@code mBuffer} is full, wait until it is consumed and there is
+     * space to write more data to it.
+     */
+    private void ensureBufferHasRemaining() {
+        if (!mBuffer.hasRemaining()) {
+            uploadBufferInternal();
+        }
+    }
+
+    /**
+     * Waits the native stack to upload data since all bytes have been written.

[pauljensen, 2016/06/10 17:45:44]

data->mBuffer's contents

[pauljensen, 2016/06/10 17:45:44]

since...->because the client has provided all bytes to be uploaded and there is
no need to wait for or expect the client to provide more bytes.

[pauljensen, 2016/06/10 17:45:44]

the->for the

[xunjieli, 2016/06/10 18:48:12]

Done.

[xunjieli, 2016/06/10 18:48:12]

Done.

[xunjieli, 2016/06/10 18:48:12]

Done.

+     */
+    private void uploadIfComplete() {
         if (mBytesWritten == mContentLength) {
             // Entire post data has been received. Now wait for network stack to
             // read it.
-            mMessageLoop.loop();
+            uploadBufferInternal();
         }
     }
 
     /**
+     * Helper function to upload the buffer to the native stack. This function

[pauljensen, 2016/06/10 17:45:44]

buffer->{@code mBuffer}

[xunjieli, 2016/06/10 18:48:12]

Done.

+     * blocks until {@code mBuffer} is consumed and there is space to write more
+     * data.
+     */
+    private void uploadBufferInternal() {
+        mBuffer.flip();
+        mMessageLoop.loop();
+    }
+
+    /**
      * Throws {@link java.net.ProtocolException} if adding {@code numBytes} will
      * exceed content length.
      */
     private void checkNotExceedContentLength(int numBytes) throws ProtocolException {
         if (mBytesWritten + numBytes > mContentLength) {
             throw new ProtocolException("expected "
                     + (mContentLength - mBytesWritten) + " bytes but received "
                     + numBytes);
         }
     }
@@ skipping 20 matching lines @@
     }
 
     private class UploadDataProviderImpl extends UploadDataProvider {
         @Override
         public long getLength() {
             return mContentLength;
         }
 
         @Override
         public void read(final UploadDataSink uploadDataSink, final ByteBuffer byteBuffer) {
-            final int availableSpace = byteBuffer.remaining();
-            if (availableSpace < mBuffer.position()) {
-                // byteBuffer does not have enough capacity, so only put a portion
-                // of mBuffer in it.
-                byteBuffer.put(mBuffer.array(), 0, availableSpace);
-                mBuffer.position(availableSpace);
-                // Move remaining buffer to the head of the buffer for use in the
-                // next read call.
-                mBuffer.compact();
-            } else {
-                // byteBuffer has enough capacity to hold the content of mBuffer.
-                mBuffer.flip();
+            if (byteBuffer.remaining() >= mBuffer.remaining()) {
                 byteBuffer.put(mBuffer);
                 // Reuse this buffer.
                 mBuffer.clear();
+                uploadDataSink.onReadSucceeded(false);
                 // Quit message loop so embedder can write more data.
                 mMessageLoop.quit();
+            } else {
+                mBuffer.position(mBuffer.position() + byteBuffer.remaining());
+                byteBuffer.put(mBuffer.array(), mBuffer.position() - byteBuffer.remaining(),
+                        byteBuffer.remaining());
+                uploadDataSink.onReadSucceeded(false);
             }
-            uploadDataSink.onReadSucceeded(false);
         }
 
         @Override
         public void rewind(UploadDataSink uploadDataSink) {
             uploadDataSink.onRewindError(
                     new HttpRetryException("Cannot retry streamed Http body", -1));
         }
     }
 
     /**
      * Sets the default buffer length for use in tests.
      */
     @VisibleForTesting
     static void setDefaultBufferLengthForTesting(int length) {
         sDefaultBufferLength = length;
     }
 }