[Cronet] Remove buffer.compact from CronetChunkedOutputStream

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

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
index 4c83bfc8fea193d986308ed20cb36fa18b963e73..6bc644bd5f84327de36f6bad02a16a0c8a3d9eb2 100644
--- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
@@ -48,11 +48,7 @@ final class CronetChunkedOutputStream extends CronetOutputStream {
 
     @Override
     public void write(int oneByte) throws IOException {
-        checkNotClosed();
-        while (mBuffer.position() == mBuffer.limit()) {
-            // Wait until buffer is consumed.
-            mMessageLoop.loop();
-        }
+        ensureBufferHasRemaining();
         mBuffer.put((byte) oneByte);
         mBytesWritten++;
     }
@@ -63,18 +59,10 @@ final class CronetChunkedOutputStream extends CronetOutputStream {
         if (buffer.length - offset < count || offset < 0 || count < 0) {
             throw new IndexOutOfBoundsException();
         }
-        if (count == 0) {
-            return;
-        }
         int toSend = count;
-        // TODO(xunjieli): refactor commond code with CronetFixedModeOutputStream.
         while (toSend > 0) {
-            if (mBuffer.position() == mBuffer.limit()) {
-                checkNotClosed();
-                // Wait until buffer is consumed.
-                mMessageLoop.loop();
-            }
-            int sent = Math.min(toSend, mBuffer.limit() - mBuffer.position());
+            ensureBufferHasRemaining();
+            int sent = Math.min(toSend, mBuffer.remaining());

[kapishnikov, 2016/07/08 23:05:12]

If toSend == mBuffer.remaining(), is it going to wait for the next chunk or
close() before uploading the data? If yes, would it be better to flip the buffer
right away and run the loop?

[xunjieli, 2016/07/11 13:40:08]

Done. That is a great suggestion. I agree that it will be better if we run the
loop right away if exactly a chunk is passed to us.

             mBuffer.put(buffer, offset + count - toSend, sent);
             toSend -= sent;
         }
@@ -84,8 +72,12 @@ final class CronetChunkedOutputStream extends CronetOutputStream {
     @Override
     public void close() throws IOException {
         super.close();
-        // Last chunk is written.
-        mLastChunk = true;
+        if (!mLastChunk) {

[kapishnikov, 2016/07/08 23:05:12]

Should we make it thread safe?

[xunjieli, 2016/07/11 13:40:08]

HttpURLConnection is not thread-safe. The APIs are supposed to be used from the
same thread. So I guess the answer is no?

+            // Consumer can only call close() when message loop is not running.
+            // Set mLastChunk to be true and flip mBuffer to upload its contents.
+            mLastChunk = true;
+            mBuffer.flip();
+        }
     }
 
     // Below are CronetOutputStream implementations:
@@ -113,27 +105,20 @@ final class CronetChunkedOutputStream extends CronetOutputStream {
 
         @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();
-                uploadDataSink.onReadSucceeded(false);
-            } else {
-                // byteBuffer has enough capacity to hold the content of mBuffer.
-                mBuffer.flip();
+            if (byteBuffer.remaining() >= mBuffer.remaining()) {

[xunjieli, 2016/07/08 20:31:00]

This is done to match the new code in CronetFixedModeOutputStream. I added a
test, which without this patch will keep uploading data to the server. The
reason is that we were previously setting
mBuffer.position(byteBuffer.remaining()) before compacting the buffer. This
causes UploadDataProvider to upload a 16kB chunk over and over again.

                 byteBuffer.put(mBuffer);
-                // Reuse this buffer.
                 mBuffer.clear();
                 uploadDataSink.onReadSucceeded(mLastChunk);
                 if (!mLastChunk) {
                     // Quit message loop so embedder can write more data.
                     mMessageLoop.quit();
                 }
+            } else {
+                int oldLimit = mBuffer.limit();
+                mBuffer.limit(mBuffer.position() + byteBuffer.remaining());
+                byteBuffer.put(mBuffer);
+                mBuffer.limit(oldLimit);
+                uploadDataSink.onReadSucceeded(false);
             }
         }
 
@@ -143,4 +128,25 @@ final class CronetChunkedOutputStream extends CronetOutputStream {
                     new HttpRetryException("Cannot retry streamed Http body", -1));
         }
     }
+
+    /**
+     * If {@code mBuffer} is full, wait until it is consumed and there is
+     * space to write more data to it.
+     */
+    private void ensureBufferHasRemaining() throws IOException {
+        if (!mBuffer.hasRemaining()) {
+            uploadBufferInternal();
+        }
+    }
+
+    /**
+     * Helper function to upload {@code mBuffer} to the native stack. This
+     * function blocks until {@code mBuffer} is consumed and there is space to
+     * write more data.
+     */
+    private void uploadBufferInternal() throws IOException {
+        checkNotClosed();
+        mBuffer.flip();
+        mMessageLoop.loop();
+    }
 }