[Cronet] Implement getOutputStream in CronetHttpURLConnection

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;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.net.ProtocolException;
+import java.nio.ByteBuffer;
+
+/**
+ * An implementation of {@link java.io.OutputStream} to send data to a server,
+ * when {@link CronetHttpURLConnection#setFixedLengthStreamingMode} is used.
+ * This implementation does not buffer the entire request body in memory.
+ * It does not support rewind. Note that {@link #write} should only be called
+ * from the thread on which the {@link #mConnection} is created.
+ */
+final class CronetFixedModeOutputStream extends OutputStream
+        implements UploadDataProvider {
+    // 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.
+    // TODO(xunjieli): figure out whether this default value should be changed.

[mef, 2015/04/06 16:14:54]

I'd suggest 32k.

[xunjieli, 2015/04/06 18:09:29]

I don't think it should be 32k. QUIC uses a upload buffer of 14520, SPDY uses
2852, and normal stream uses 16384. 32k is bigger than all of them. I think we
can probably use 16384, but I am not sure.

+    @VisibleForTesting
+    private static int sDefaultBufferLength = 2048;
+    private final CronetHttpURLConnection mConnection;
+    private final MessageLoop mMessageLoop;
+    private final long mContentLength;
+    private final ByteBuffer mBuffer;
+    private long mBytesReceived;
+
+    /**
+     * Package protected constructor.
+     * @param connection The CronetHttpURLConnection object.
+     * @param contentLength The content length of the request body. Non-zero for
+     *            non-chunked upload.
+     */
+    CronetFixedModeOutputStream(CronetHttpURLConnection connection,
+            long contentLength, MessageLoop messageLoop) {
+        if (connection == null) {
+            throw new NullPointerException();
+        }
+        if (contentLength < 0) {
+            throw new IllegalArgumentException(
+                    "Content length must be larger than 0 for non-chunked upload.");
+        }
+        mContentLength = contentLength;
+        int bufferSize = (int) Math.min(mContentLength, sDefaultBufferLength);
+        mBuffer = ByteBuffer.allocate(bufferSize);
+        mConnection = connection;
+        mMessageLoop = messageLoop;
+        mBytesReceived = 0;
+    }
+
+    @Override
+    public void write(int oneByte) throws IOException {
+        checkNotExceedContentLength(1);
+        while (mBuffer.position() == mBuffer.limit()) {
+            // Wait until buffer is consumed.
+            mMessageLoop.loop();
+        }
+        mBuffer.put((byte) oneByte);
+        mBytesReceived++;

[mef, 2015/04/06 16:14:54]

Rename mBytesReceived => mBytesWritten?
Not sure about java code style guide, but Chromium C++ suggests prefix ++ like
in ++mBytesReceived;

[xunjieli, 2015/04/06 18:09:29]

Done. I believe the increment should be after the variable in Java. I haven't
seen the other variant that much. Looks like the same goes to the java files in
chromium:
https://code.google.com/p/chromium/codesearch#search/&q=file:java$%20%22%2B%2...

[mef, 2015/04/06 18:37:18]

sg

+        if (mBytesReceived == mContentLength) {
+            // Entire post data has been received. Now wait for network stack to
+            // read it.
+            mMessageLoop.loop();
+        }
+    }
+
+    @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());
+            mBuffer.put(buffer, offset + count - toSend, sent);
+            toSend -= sent;
+        }
+        mBytesReceived += count;
+        if (mBytesReceived == mContentLength) {
+            // Entire post data has been received. Now wait for network stack to
+            // read it.
+            mMessageLoop.loop();
+        }
+    }
+
+    /**
+     * Checks whether content received is less than Content-Length.
+     */
+    void checkReceivedEnoughContent() throws IOException {

[mef, 2015/04/06 16:14:54]

not used?

[xunjieli, 2015/04/06 18:09:29]

Used in CronetHttpURLConnection's startRequest() to make sure we have received
enough content.

[mef, 2015/04/06 18:37:18]

Ah, I see, maybe add comment? Not sure.

[xunjieli, 2015/04/06 21:03:45]

Done. Right, this should deserve a comment.

+        if (mBytesReceived < mContentLength) {
+            throw new ProtocolException("Content received is less than Content-Length.");
+        }
+    }
+
+    // TODO(xunjieli): implement close().
+
+    /**
+     * Throws {@link java.net.ProtocolException} if adding {@code numBytes} will
+     * exceed content length.
+     */
+    private void checkNotExceedContentLength(int numBytes) throws ProtocolException {
+        if (mContentLength != -1 && mBytesReceived + numBytes > mContentLength) {

[mef, 2015/04/06 16:14:54]

When mContentLength could == -1?

[xunjieli, 2015/04/06 18:09:29]

Done. Good catch!

+            throw new ProtocolException("expected "
+                    + (mContentLength - mBytesReceived) + " bytes but received "
+                    + numBytes);
+        }
+    }
+
+    // Below are UploadDataProvider implementations. Only intended to be used
+    // within Cronet.
+
+    @Override
+    public long getLength() {
+        return mContentLength;
+    }
+
+    @Override
+    public void read(final UploadDataSink uploadDataSink, final ByteBuffer byteBuffer) {
+        int availableSpace = byteBuffer.capacity() - byteBuffer.position();
+        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();

[mef, 2015/04/06 16:14:54]

moving memory is not cheap, is there a way to avoid it?

[xunjieli, 2015/04/06 18:09:29]

I don't think it will be a problem if our buffer size is 2048, since it is
smaller than all buffer sizes used in Chromium (QUIC: 14520, SPDY: 2852, Normal:
16384), the code will use the else clause. However, if we make the buffer size
bigger (say 16k or 32k), we will run into this code path.

+        } else {
+            // byteBuffer has enough capacity to hold the content of mBuffer.
+            mBuffer.flip();
+            byteBuffer.put(mBuffer);
+            // Reuse this buffer.
+            mBuffer.clear();
+            // Quit message loop so embedder can write more data.
+            mMessageLoop.postQuitTask();
+        }
+        uploadDataSink.onReadSucceeded(false);
+    }
+
+    @Override
+    public void rewind(UploadDataSink uploadDataSink) {
+        uploadDataSink.onRewindError(new IllegalStateException(
+                "Rewind is not supported by CronetFixedModeOutputStream."));
+    }
+
+    /**
+     * Sets the default buffer length for use in tests.
+     */
+    @VisibleForTesting
+    static void setDefaultBufferLengthForTesting(int length) {
+        sDefaultBufferLength = length;
+    }
+}