[Cronet] Implement getOutputStream in CronetHttpURLConnection

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

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
new file mode 100644
index 0000000000000000000000000000000000000000..878d21b2455916b50857fa1fcedd906ecb4a153a
--- /dev/null
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
@@ -0,0 +1,153 @@
+// 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;
+import java.io.OutputStream;
+import java.net.ProtocolException;
+import java.nio.ByteBuffer;
+
+/**
+ * An implementation of {@link java.io.OutputStream} that buffers entire request
+ * body in memory. This is used when neither
+ * {@link CronetHttpURLConnection#setFixedLengthStreamingMode}
+ * nor {@link CronetHttpURLConnection#setChunkedStreamingMode} is set.
+ */
+final class CronetBufferedOutputStream extends OutputStream
+        implements UploadDataProvider {
+    // If content length is not passed in the constructor, this is -1.
+    private final int mInitialContentLength;
+    private final CronetHttpURLConnection mConnection;
+    // Internal buffer that is used to buffer the request body.
+    private ByteBuffer mBuffer;
+    private boolean mConnected = false;
+    private boolean mFirstReadInitiated = false;
+
+    /**
+     * Package protected constructor.
+     * @param connection The CronetHttpURLConnection object.
+     * @param contentLength The content length of the request body. It must not
+     *            be smaller than 0 or bigger than {@link Integer.MAX_VALUE}.
+     */
+    CronetBufferedOutputStream(final CronetHttpURLConnection connection,
+            final long contentLength) {
+        if (connection == null) {
+            throw new NullPointerException();

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

add error message?

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

Done.

+        }
+
+        if (contentLength > Integer.MAX_VALUE) {
+            throw new IllegalStateException("Use setFixedLengthStreamingMode()"

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

I would throw IllegalArgumentException here for consistency with check below,
and considering that this is caller-provided argument.

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

Done.

+                + " or setChunkedStreamingMode() for requests larger than 2GB.");
+        }
+        if (contentLength < 0) {
+            throw new IllegalArgumentException("Content length < 0.");
+        }
+        mConnection = connection;
+        mInitialContentLength = (int) contentLength;
+        mBuffer = ByteBuffer.allocate(mInitialContentLength);
+    }
+
+    /**
+     * Package protected constructor used when content length is not known.
+     * @param connection The CronetHttpURLConnection object.
+     */
+    CronetBufferedOutputStream(final CronetHttpURLConnection connection) {
+        if (connection == null) {
+            throw new NullPointerException();
+        }
+
+        mConnection = connection;
+        mInitialContentLength = -1;
+        // Buffering without knowing content-length.
+        mBuffer = ByteBuffer.allocate(2048);

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

Define 2048 as constant? While there, should it be like 32k?

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

Hmm.. 2048 isn't used anywhere else in this file, so I am not sure whether that
should a constant. The value does not matter a whole lot, since it will grow
eventually if bytes passed in don't fit.

I don't think it should be 32k. QUIC upload buffer : 14520, SPDY = 2852, and
Normal: 16384. 32k is larger than all of them.

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

16k sgtm. My concern is the need for reallocs/copies if data size exceeds buffer
size.

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

Done. I don't know which one is better. I guess we could try with 16384 first.

+    }
+
+    @Override
+    public void write(int oneByte) throws IOException {
+        ensureCanWrite(1);
+        mBuffer.put((byte) oneByte);
+    }
+
+    @Override
+    public void write(byte[] buffer, int offset, int count) throws IOException {
+        ensureCanWrite(count);
+        mBuffer.put(buffer, offset, count);
+    }
+
+    /**
+     * Sets {@link #mConnected} to {@code true}.
+     */
+    void setConnected() throws IOException {
+        mConnected = true;
+        if (mBuffer.position() < mInitialContentLength) {
+            throw new ProtocolException("Content received is less than Content-Length");
+        }
+        // Flip the buffer to prepare it for UploadDataProvider read calls.
+        mBuffer.flip();
+    }
+
+    // TODO(xunjieli): implement close().
+
+    /**
+     * Ensures that {@code count} bytes can be written to the internal buffer.
+     */
+    private void ensureCanWrite(int count) throws IOException {
+        if (mInitialContentLength != -1
+                && mBuffer.position() + count > mInitialContentLength) {
+            // Error message is to match that of the default implementation.
+            throw new ProtocolException("exceeded content-length limit of "
+                    + mInitialContentLength + " bytes");
+        }
+        if (mConnected) {
+            throw new IllegalStateException("Cannot write after being connected.");
+        }
+        if (mInitialContentLength != -1 || mBuffer.limit() - mBuffer.position() > count) {

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

Can we split it into 2 separate checks? 
Also maybe combine it with first 'if mInitialContentLength != -1', to explicitly
handle buffer that shouldn't grow?

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

Done.

+            // If mInitialContentLength is known or there is enough capacity,
+            // the buffer should not grow.
+            return;
+        }
+        int afterSize = Math.max(mBuffer.capacity() * 2, mBuffer.capacity() + count);
+        ByteBuffer newByteBuffer = ByteBuffer.allocate(afterSize);
+        mBuffer.flip();
+        newByteBuffer.put(mBuffer);
+        mBuffer = newByteBuffer;
+    }
+
+    // Below are UploadDataProvider implementations. Only intended to be used
+    // within Cronet.
+
+    @Override
+    public long getLength() {
+        // This method is supposed to be called just before starting the request.
+        // If content length is not initially passed in, the number of bytes
+        // written will be used as the content length.
+        if (mInitialContentLength == -1) {
+            return mBuffer.position();

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

hrm, this will be 0 if rewind() is called.

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

getLength() is called in CronetUploadDataStream's constructor (before rewind()
is called), so it should not matter. I have tests
(CronetBuffferedOutputStream#testRewind and #testRewindWithoutContentLength) to
make sure it works.

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

My concern here is fragility, few years from now somebody will call getLength()
from another place and will be surprised by results.

Can the limit() be used here instead?

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

If we have a buffer of 2048 bytes, and we write 18 bytes, the position will be
18, but the limit is 2048, so we can't use limit().
I hear you. It is not the best design. I don't have a better design on top of my
mind. I need to think about it. Added a TODO.

+        }
+        return mInitialContentLength;
+    }
+
+    @Override
+    public void read(UploadDataSink uploadDataSink, ByteBuffer byteBuffer) {
+        int oldPos = byteBuffer.position();
+        int availableSpace = byteBuffer.capacity() - byteBuffer.position();

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

Maybe rename availableSpace => readSize?

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

It's actually not the read size, since the else clause writes the whole of
mBuffer, so I'd keep it as the availableSpace of the byteBuffer.

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

ok

+        if (availableSpace  < mBuffer.limit() - mBuffer.position()) {
+            byteBuffer.put(mBuffer.array(), mBuffer.position(), availableSpace);
+            mBuffer.position(mBuffer.position() + availableSpace);
+        } else {
+            byteBuffer.put(mBuffer);
+        }
+        uploadDataSink.onReadSucceeded(false);
+    }
+
+    @Override
+    public void rewind(UploadDataSink uploadDataSink) {
+        mBuffer.position(0);
+        uploadDataSink.onRewindSucceeded();
+    }
+}