[Cronet] Implement getOutputStream in CronetHttpURLConnection

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

 // Copyright 2014 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 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;
 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;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
 
 /**
  * An implementation of HttpURLConnection that uses Cronet to send requests and
  * receive response. This class inherits a {@code connected} field from the
  * superclass. That field indicates whether a connection has ever been
  * 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;
         mMessageLoop = new MessageLoop();
         mRequest = mUrlRequestContext.createRequest(url.toString(),
                 new CronetUrlRequestListener(), mMessageLoop);
         mInputStream = new CronetInputStream(this);
         mRequestHeaders = new ArrayList<Pair<String, String>>();
     }
 
     /**
      * 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();
     }
 
     /**
      * Releases this connection so that its resources may be either reused or
      * closed.
      */
     @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) {
                 e.printStackTrace();
             }
             mInputStream = null;
             mRequest.cancel();
         }
     }
 
     /**
      * Returns the response message returned by the remote HTTP server.
      */
     @Override
     public String getResponseMessage() throws IOException {
-        connect();
+        getResponse();
         return mResponseInfo.getHttpStatusText();
     }
 
     /**
      * Returns the response code returned by the remote HTTP server.
      */
     @Override
     public int getResponseCode() throws IOException {
-        connect();
+        getResponse();
         return mResponseInfo.getHttpStatusCode();
     }
 
     /**
      * Returns an unmodifiable map of the response-header fields and values.
      */
     @Override
     public Map<String, List<String>> getHeaderFields() {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return Collections.emptyMap();
         }
         return mResponseInfo.getAllHeaders();
     }
 
     /**
      * Returns the value of the named header field. If called on a connection
      * that sets the same header multiple times with possibly different values,
      * only the last value is returned.
      */
     @Override
     public final String getHeaderField(String fieldName) {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }
         Map<String, List<String>> map = mResponseInfo.getAllHeaders();
         if (!map.containsKey(fieldName)) {
             return null;
         }
         List<String> values = map.get(fieldName);
         return values.get(values.size() - 1);
     }
@@ skipping 28 matching lines @@
      * Returns an InputStream for reading data from the resource pointed by this
      * URLConnection.
      * @throws FileNotFoundException if http response code is equal or greater
      *             than {@link HTTP_BAD_REQUEST}.
      * @throws IOException If the request gets a network error or HTTP error
      *             status code, or if the caller tried to read the response body
      *             of a redirect when redirects are disabled.
      */
     @Override
     public InputStream getInputStream() throws IOException {
-        connect();
+        getResponse();
         if (!instanceFollowRedirects && mOnRedirectCalled) {
             throw new IOException("Cannot read response body of a redirect.");
         }
         // Emulate default implementation's behavior to throw
         // FileNotFoundException when we get a 400 and above.
         if (mResponseInfo.getHttpStatusCode() >= HTTP_BAD_REQUEST) {
             throw new FileNotFoundException(url.toString());
         }
         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.
      */
     @Override
     public InputStream getErrorStream() {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }
         if (mResponseInfo.getHttpStatusCode() >= HTTP_BAD_REQUEST) {
             return mInputStream;
         }
         return null;
     }
 
     /**
@@ skipping 75 matching lines @@
     /**
      * Returns whether this connection uses a proxy server.
      */
     @Override
     public boolean usingProxy() {
         // TODO(xunjieli): implement this.
         return false;
     }
 
     /**
+     * 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.
      */
     ByteBuffer getMoreData() throws IOException {
         mResponseByteBuffer = null;
         mMessageLoop.loop();
         return mResponseByteBuffer;
     }
 
@@ skipping 73 matching lines @@
          */
         private void setResponseDataCompleted() {
             if (mInputStream != null) {
                 mInputStream.setResponseDataCompleted();
             }
             mMessageLoop.postQuitTask();
         }
     }
 
     /**
+     * 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) {
             throw new NullPointerException(
                     "Response info is null when there is no exception.");
         }
     }
 
     /**
      * Helper method to return the response header field at position pos.
      */
     private Pair<String, String> getHeaderFieldPair(int pos) {
         try {
-            connect();
+            getResponse();
         } catch (IOException e) {
             return null;
         }
         List<Pair<String, String>> headers =
                 mResponseInfo.getAllHeadersAsList();
         if (pos >= headers.size()) {
             return null;
         }
         return headers.get(pos);
     }
 }