[Cronet] Implement getOutputStream in CronetHttpURLConnection

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

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
index 585b6e6af7c75855eac1b29dae12a5e015f4fae0..f149916fa407f53490c30eb6fc8ffdecee606bff 100644
--- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
+++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
@@ -4,10 +4,12 @@
 
 package org.chromium.net.urlconnection;
 
+import android.util.Log;
 import android.util.Pair;
 
 import org.chromium.net.ExtendedResponseInfo;
 import org.chromium.net.ResponseInfo;
+import org.chromium.net.UploadDataProvider;
 import org.chromium.net.UrlRequest;
 import org.chromium.net.UrlRequestContext;
 import org.chromium.net.UrlRequestException;
@@ -16,8 +18,10 @@ import org.chromium.net.UrlRequestListener;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
+import java.net.ProtocolException;
 import java.net.URL;
 import java.nio.ByteBuffer;
 import java.util.ArrayList;
@@ -33,18 +37,21 @@ import java.util.TreeMap;
  * attempted.
  */
 public class CronetHttpURLConnection extends HttpURLConnection {
+    private static final String TAG = "CronetHttpURLConnection";
     private final UrlRequestContext mUrlRequestContext;
     private final MessageLoop mMessageLoop;
     private final UrlRequest mRequest;
     private final List<Pair<String, String>> mRequestHeaders;
 
     private CronetInputStream mInputStream;
+    private OutputStream mOutputStream;
     private ResponseInfo mResponseInfo;
     private UrlRequestException mException;
     private ByteBuffer mResponseByteBuffer;
     private boolean mOnRedirectCalled = false;
+    private boolean mHasResponse = false;
 
-    protected CronetHttpURLConnection(URL url,
+    public CronetHttpURLConnection(URL url,
             UrlRequestContext urlRequestContext) {
         super(url);
         mUrlRequestContext = urlRequestContext;
@@ -58,26 +65,11 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     /**
      * Opens a connection to the resource. If the connect method is called when
      * the connection has already been opened (indicated by the connected field
-     * having the value true), the call is ignored unless an exception is thrown
-     * previously, in which case, the exception will be rethrown.
+     * having the value true), the call is ignored.
      */
     @Override
     public void connect() throws IOException {
-        if (connected) {
-            checkHasResponse();
-            return;
-        }
-        connected = true;
-        for (Pair<String, String> requestHeader : mRequestHeaders) {
-            mRequest.addHeader(requestHeader.first, requestHeader.second);
-        }
-        if (!getUseCaches()) {
-            mRequest.disableCache();
-        }
-        mRequest.start();
-        // Blocks until onResponseStarted or onFailed is called.
-        mMessageLoop.loop();
-        checkHasResponse();
+        startRequest();
     }
 
     /**
@@ -87,7 +79,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     @Override
     public void disconnect() {
         // Disconnect before connection is made should have no effect.
-        if (connected) {
+        if (connected && mInputStream != null) {
             try {
                 mInputStream.close();
             } catch (IOException e) {
@@ -103,7 +95,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
      */
     @Override
     public String getResponseMessage() throws IOException {
-        connect();
+        getResponse();
         return mResponseInfo.getHttpStatusText();
     }
 
@@ -112,7 +104,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
      */
     @Override
     public int getResponseCode() throws IOException {
-        connect();
+        getResponse();
         return mResponseInfo.getHttpStatusCode();
     }
 
@@ -122,7 +114,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     @Override
     public Map<String, List<String>> getHeaderFields() {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return Collections.emptyMap();
         }
@@ -137,7 +129,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     @Override
     public final String getHeaderField(String fieldName) {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }
@@ -186,7 +178,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
      */
     @Override
     public InputStream getInputStream() throws IOException {
-        connect();
+        getResponse();
         if (!instanceFollowRedirects && mOnRedirectCalled) {
             throw new IOException("Cannot read response body of a redirect.");
         }
@@ -198,6 +190,99 @@ public class CronetHttpURLConnection extends HttpURLConnection {
         return mInputStream;
     }
 
+    @Override
+    public OutputStream getOutputStream() throws IOException {
+        if (mOutputStream == null) {
+            if (connected) {
+                throw new ProtocolException(
+                        "Cannot write to OutputStream after receiving response.");
+            }
+            long fixedStreamingModeContentLength = getStreamingModeContentLength();
+            if (fixedStreamingModeContentLength != -1) {
+                mOutputStream = new CronetFixedModeOutputStream(this,
+                        fixedStreamingModeContentLength, mMessageLoop);
+                // Start the request now since all headers can be sent.

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

why is this true? 

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

Because the user sets fixedLengthStreamingMode, so we know we can set the
content length header.
For the case where fixedLengthStreamingMode is not set, the request headers
should only be sent after all content has been buffered into memory.

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

Yes, but why couldn't user call setRequestProperty() AFTER calling
getOutputStream()? In other words, why call to getOutputStream should start the
request?

BTW, have you seen this:

http://docs.oracle.com/javase/7/docs/api/java/net/HttpURLConnection.html#setF...

A HttpRetryException will be thrown when reading the response if authentication
or redirection are required

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

getOutputStream() should try to establish a connection to the server for writing
according to its specs. However, in the case where setFixedLengthStreamingMode
is not used, connection establishment is delayed. Personally I find the
intricacies of this API is a little confusing.
http://stackoverflow.com/questions/10116961/can-you-explain-the-httpurlconnec...
helps to understand.
Done. Thanks for bringing this up. Changed the error in rewind below to match
this, also updated test.

+                startRequest();
+            } else {
+                // For the buffered case, start the request only when
+                // content-length bytes are received, or when a
+                // connect action is initiated by the consumer.
+                Log.d(TAG, "Outputstream is being buffered in memory.");
+                String length = getRequestProperty("Content-Length");
+                if (length == null) {
+                    mOutputStream = new CronetBufferedOutputStream(this);
+                } else {
+                    long lengthParsed = Long.parseLong(length);

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

what if it is over 2gb?

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

CronetBufferedOutputStream's constructor will throw an illegal argument
exception.

+                    mOutputStream = new CronetBufferedOutputStream(this, lengthParsed);
+                }
+            }
+        }
+        return mOutputStream;
+    }
+
+    /**
+     * Helper method to get content length passed in by
+     * {@link #setFixedLengthStreamingMode}
+     */
+    private long getStreamingModeContentLength() {
+        long contentLength = fixedContentLength;
+        // Use reflection to see whether fixedContentLengthLong (only added
+        // in API 19) is inherited.
+        try {
+            Class<?> parent = this.getClass();
+            long superFixedContentLengthLong =
+                    parent.getField("fixedContentLengthLong").getLong(this);
+            if (superFixedContentLengthLong != -1) {
+                contentLength = superFixedContentLengthLong;
+            }
+        } catch (Exception e) {
+            // Ignored.
+        }
+        return contentLength;
+    }
+
+    /**
+     * Starts the request if {@code connected} is false.
+     */
+    private void startRequest() throws IOException {
+        if (connected) {
+            return;
+        }
+        if (doOutput) {
+            if (mOutputStream != null) {
+                mRequest.setUploadDataProvider(
+                        (UploadDataProvider) mOutputStream, mMessageLoop);
+                if (getRequestProperty("Content-Length") == null) {
+                    addRequestProperty("Content-Length",
+                            Long.toString(((UploadDataProvider) mOutputStream).getLength()));
+                }
+                if (mOutputStream instanceof CronetBufferedOutputStream) {
+                    // Disallow the embedder to write more data, and prepare
+                    // internal buffer for reading.
+                    ((CronetBufferedOutputStream) mOutputStream).setConnected();

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

can mOutputStream be a common base class like 'CronetOutputStream' that has
'setConnected' method, so you wouldn't need to cast?

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

Only CronetBufferedOutputStream needs this method. For the case of
CronetFixedModeOutputStream, request is started (connected) when the output
stream is constructed. 

+                }
+            } else {
+                if (getRequestProperty("Content-Length") == null) {

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

suggest string constant for "Content-Length" and "Content-Type".

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

Done.

+                    addRequestProperty("Content-Length", "0");
+                }
+            }
+            // Default Content-Type to application/x-www-form-urlencoded
+            if (getRequestProperty("Content-Type") == null) {
+                addRequestProperty("Content-Type",
+                        "application/x-www-form-urlencoded");
+            }
+        }
+        for (Pair<String, String> requestHeader : mRequestHeaders) {
+            mRequest.addHeader(requestHeader.first, requestHeader.second);
+        }
+        if (!getUseCaches()) {
+            mRequest.disableCache();
+        }
+        connected = true;
+        // Start the request.
+        mRequest.start();
+    }
+
     /**
      * Returns an input stream from the server in the case of an error such as
      * the requested file has not been found on the remote server.
@@ -205,7 +290,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     @Override
     public InputStream getErrorStream() {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }
@@ -301,6 +386,15 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     }
 
     /**
+     * Sets chunked streaming mode.
+     */
+    @Override
+    public void setChunkedStreamingMode(int chunklen) {
+        // TODO(xunjieli): implement this.
+        throw new UnsupportedOperationException("Chunked mode not supported yet");
+    }
+
+    /**
      * Used by {@link CronetInputStream} to get more data from the network
      * stack. This should only be called after the request has started. Note
      * that this call might block if there isn't any more data to be read.
@@ -394,11 +488,30 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     }
 
     /**
+     * Blocks until the respone headers are received.
+     */
+    private void getResponse() throws IOException {
+        // Check to see if enough data has been received. CronetBufferedOutputStream's
+        // case is checked in CronetBufferedOutputStream#setConnected().
+        if (mOutputStream != null && mOutputStream instanceof CronetFixedModeOutputStream) {
+            ((CronetFixedModeOutputStream) mOutputStream).checkReceivedEnoughContent();

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

Maybe make checkReceivedEnoughContent a base class method as well?

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

Only CronetFixedModeOutputStream needs this method. CronetBufferedOutputStream's
check is in setConnected. These two classes already have a base class
OutputStream. Java does not allow multiple inheritance. If we were to make a
base class for these two, that base class has to implement methods in
OutputStream. However these two classes shouldn't share the common write
methods, for example.

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

But it can be an abstract class that extends OutputStream but doesn't override
any of its methods, right?

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

Done. Yes, you are right. Changed the code to do that, although
checkReceivedEnoughContent is only applicable to one class and not the other,
same goes to setConnected. I guess grouping them in the same place is cleaner
nevertheless.

+        }
+        if (!mHasResponse) {
+            startRequest();
+            // Blocks until onResponseStarted or onFailed is called.
+            mMessageLoop.loop();
+            mHasResponse = true;
+        }
+        checkHasResponse();
+    }
+
+    /**
      * Checks whether response headers are received, and throws an exception if
      * an exception occurred before headers received. This method should only be
      * called after onResponseStarted or onFailed.
      */
     private void checkHasResponse() throws IOException {
+        if (!mHasResponse) throw new IllegalStateException("No response.");

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

isn't it a duplicate of mResponseInfo == null?

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

Nope. mResponseInfo can be null when the request failed.

         if (mException != null) {
             throw mException;
         } else if (mResponseInfo == null) {
@@ -412,7 +525,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
      */
     private Pair<String, String> getHeaderFieldPair(int pos) {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }