[Cronet] Remove buffer.compact from CronetChunkedOutputStream

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.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.net.UploadDataProvider;
 import org.chromium.net.UploadDataSink;
 
 import java.io.IOException;
@@ skipping 11 matching lines @@
     private final CronetHttpURLConnection mConnection;
     private final MessageLoop mMessageLoop;
     private final ByteBuffer mBuffer;
     private final UploadDataProvider mUploadDataProvider = new UploadDataProviderImpl();
     private long mBytesWritten;
     private boolean mLastChunk = false;
 
     /**
      * Package protected constructor.
      * @param connection The CronetHttpURLConnection object.
      * @param contentLength The content length of the request body. Non-zero for

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

contentLength  => chunkLength

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

Done. Good catch!

      *            non-chunked upload.
      */
     CronetChunkedOutputStream(
             CronetHttpURLConnection connection, int chunkLength, MessageLoop messageLoop) {
         if (connection == null) {
             throw new NullPointerException();
         }
         if (chunkLength <= 0) {
             throw new IllegalArgumentException("chunkLength should be greater than 0");
         }
         mBuffer = ByteBuffer.allocate(chunkLength);
         mConnection = connection;
         mMessageLoop = messageLoop;
         mBytesWritten = 0;
     }
 
     @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++;
     }
 
     @Override
     public void write(byte[] buffer, int offset, int count) throws IOException {
         checkNotClosed();
         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;
         }
         mBytesWritten += count;
     }
 
     @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:
 
     @Override
     void setConnected() throws IOException {
         // Do nothing.
     }
 
     @Override
     void checkReceivedEnoughContent() throws IOException {
         // Do nothing.
     }
 
     @Override
     UploadDataProvider getUploadDataProvider() {
         return mUploadDataProvider;
     }
 
     private class UploadDataProviderImpl extends UploadDataProvider {
         @Override
         public long getLength() {
             return -1;
         }
 
         @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);
             }
         }
 
         @Override
         public void rewind(UploadDataSink uploadDataSink) {
             uploadDataSink.onRewindError(
                     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();
+    }
 }