Add HttpUrlConnection backed implementation of CronetEngine.

components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java

Index: components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
diff --git a/components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java b/components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
new file mode 100644
index 0000000000000000000000000000000000000000..b0094f8e59bf244108a144217808ba6cd2656a23
--- /dev/null
+++ b/components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.java
@@ -0,0 +1,838 @@
+// 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;
+
+import android.annotation.TargetApi;
+import android.net.TrafficStats;
+import android.os.Build;
+import android.util.Log;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.net.URI;
+import java.net.URL;
+import java.nio.ByteBuffer;
+import java.nio.channels.Channels;
+import java.nio.channels.ReadableByteChannel;
+import java.nio.channels.WritableByteChannel;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.concurrent.Executor;
+import java.util.concurrent.RejectedExecutionException;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Pure java UrlRequest, backed by {@link HttpURLConnection}.
+ */
+@TargetApi(15)

[pauljensen, 2015/12/08 19:49:53]

Can you add a comment why 15 is necessary and can you replace this with a
constant (e.g. Build.VERSION_CODES.JELLY_BEAN_MR1)?

[Charles, 2015/12/11 16:45:39]

Is that the style in chrome? I tend to use integer literals because that's
what's specified in the API documentation - "Added in API leve 11"

[pauljensen, 2015/12/14 20:02:05]

I find a pre-defined constant is always preferable to an integer literal. 
Please also add a comment about why this is necessary.

[Charles, 2015/12/15 22:32:35]

Done.

+final class JavaUrlRequest implements UrlRequest {
+    private static final String X_ANDROID_SELECTED_TRANSPORT = "X-Android-Selected-Transport";

[pauljensen, 2015/12/08 19:49:53]

Does this header exist?  According to OkHttp changelog it was replace with
OkHttp-Selected-Transport which was in turn later replaced with
OkHttp-Selected-Protocol:
http://androidxref.com/6.0.0_r1/xref/external/okhttp/CHANGELOG.md

[Charles, 2015/12/11 16:45:40]

It was in here at some point:
https://android.googlesource.com/platform/external/okhttp/+/00834c9a00d53b29c...

[pauljensen, 2015/12/14 20:02:05]

I agree it was there at some point, but it isn't anymore AFAIK.

[Charles, 2015/12/15 22:32:35]

Old android code doesn't go away, it's still running on users devices for years.

[pauljensen, 2015/12/17 19:59:49]

That may be so, but newer Android releases will fail here and need to be fixed.

[Charles, 2015/12/18 23:37:51]

Tested.

[pauljensen, 2015/12/21 19:31:24]

I assume that means the test passed on Marshmallow?
It looks like Marshmallow may override this setting here:
http://androidxref.com/6.0.0_r1/xref/external/okhttp/android/main/java/com/sq...

+    private static final String TAG = "JavaUrlConnection";
+    private static final int DEFAULT_UPLOAD_BUFFER_SIZE = 8192;
+    private static final int DEFAULT_CHUNK_LENGTH = DEFAULT_UPLOAD_BUFFER_SIZE;
+    private final AsyncUrlRequestCallback mCallbackAsync;
+    private final Executor mExecutor;
+    private final String mUserAgent;
+    private final Map<CaseInsensitiveString, String> mRequestHeaders = new HashMap<>();
+    private final List<String> mUrlChain = new ArrayList<>();
+    /**
+     * This is the source of thread safety in this class - no other synchronization is performed.
+     * By compare-and-swapping from one state to another, we guarantee that operations aren't
+     * running concurrently. Only the winner of a CAS proceeds.
+     *
+     * <p>A caller can lose a CAS for three reasons - user error (two calls to read() without
+     * waiting for the read to succeed), runtime error (network code or user code throws an
+     * exception), or cancellation.
+     */
+    private final AtomicReference<State> mState = new AtomicReference<>(State.NOT_STARTED);
+    private final int mTrafficStatsTag;

[pauljensen, 2015/12/08 19:49:53]

Can we add a comment explaining that we retag requests for the thread they were
created on, to avoid having them all accounted for on our thread pool?
Would be nice if we could also add a test for this.

[Charles, 2015/12/11 16:45:39]

Unfortunately there's no way to externally observe that data info was sent to a
specific tag (or I'd be using it to populate the bytes transferred). 

I had the idea of creating a wrapper HttpUrlConnection that checks the current
tag, but that's pretty complex and I'd rather do it in another CL.

[pauljensen, 2015/12/14 20:02:05]

Ok, I guess there isn't an easy way to test this, but let's add the comment
about why/how this code exists.

[Charles, 2015/12/15 22:32:35]

Done.

+
+    /* These don't change with redirects */
+    private String mInitialMethod;
+    private UploadDataProvider mUploadDataProvider;
+    private Executor mUploadExecutor;
+    /**
+     *  Holds a subset of StatusValues - {@link State#STARTED} can represent
+     * {@link Status#SENDING_REQUEST} or {@link Status#WAITING_FOR_RESPONSE}. While the distinction
+     * isn't needed to implement the logic in this class, it is needed to implement
+     * {@link #getStatus(StatusListener)}.
+     *
+     * <p>Concurrency notes - this value is not atomically updated with mState, so there is some
+     * risk
+     * that we'd get an inconsistent snapshot of both - however, it also happens that this value is
+     * only used with the STARTED state, so it's inconsequential.
+     */
+    @Status.StatusValues private volatile int mAdditionalStatusDetails = Status.INVALID;
+
+    /* These change with redirects. */
+    private String mCurrentUrl;
+    private ReadableByteChannel mResponseChannel;
+    private UrlResponseInfo mUrlResponseInfo;
+    private String mPendingRedirectUrl;
+    /**
+     * The happens-before edges created by the executor submission and AtomicReference setting are
+     * sufficient to guarantee the correct behavior of this field; however, this is an
+     * AtomicReference so that we can cleanly dispose of a new connection if we're cancelled during
+     * a redirect, which requires get-and-set semantics.
+     * */
+    private final AtomicReference<HttpURLConnection> mCurrentUrlConnection =
+            new AtomicReference<>();
+
+    /**
+     * NOT_STARTED -> STARTED -> REDIRECTED -> STARTED -> AWAITING_READ -> READING -> AWAITING_READ
+     * -> COMPLETE

[pauljensen, 2015/12/08 19:49:52]

This might be easier to read if it contained loops, then again ASCII art is
awful:

             /- REDIRECTED <-\  /---- READING <----\
             |               |  |                  |
             \               /  \                  /
NOT_STARTED ---> STARTED ---------> AWAITING_READ ---> COMPLETE

[Charles, 2015/12/11 16:45:39]

Done.

+     */
+    private enum State {
+        NOT_STARTED,
+        STARTED,
+        REDIRECTED,
+        AWAITING_READ,
+        READING,
+        ERROR,
+        COMPLETE,
+        CANCELLED,
+    }
+
+    JavaUrlRequest(Callback callback, final Executor executor, Executor userExecutor, String url,

[pauljensen, 2015/12/08 19:49:52]

this could really use a comment explaining what arguments are, at least the
first two

[Charles, 2015/12/11 16:45:39]

Done.

+            String userAgent) {
+        if (url == null) {
+            throw new NullPointerException("URL is required");
+        }
+        if (callback == null) {
+            throw new NullPointerException("Listener is required");
+        }
+        if (executor == null) {
+            throw new NullPointerException("Executor is required");
+        }
+        if (userExecutor == null) {
+            throw new NullPointerException("Executor is required");
+        }
+        this.mCallbackAsync = new AsyncUrlRequestCallback(callback, userExecutor);
+        this.mTrafficStatsTag = TrafficStats.getThreadStatsTag();
+        this.mExecutor = new Executor() {
+            @Override
+            public void execute(final Runnable command) {
+                executor.execute(new Runnable() {
+                    @Override
+                    public void run() {
+                        int oldTag = TrafficStats.getThreadStatsTag();
+                        TrafficStats.setThreadStatsTag(mTrafficStatsTag);
+                        try {
+                            command.run();
+                        } finally {
+                            TrafficStats.setThreadStatsTag(oldTag);
+                        }
+                    }
+                });
+            }
+        };
+        this.mCurrentUrl = url;
+        this.mUserAgent = userAgent;
+    }
+
+    @Override
+    public void setHttpMethod(String method) {
+        checkNotStarted();
+        if (method == null) {
+            throw new NullPointerException("Method is required.");
+        }
+        if ("OPTIONS".equalsIgnoreCase(method) || "GET".equalsIgnoreCase(method)
+                || "HEAD".equalsIgnoreCase(method) || "POST".equalsIgnoreCase(method)
+                || "PUT".equalsIgnoreCase(method) || "DELETE".equalsIgnoreCase(method)
+                || "TRACE".equalsIgnoreCase(method) || "PATCH".equalsIgnoreCase(method)) {
+            mInitialMethod = method;
+        } else {
+            throw new IllegalArgumentException("Invalid http method " + method);
+        }
+    }
+
+    private void checkNotStarted() {
+        State state = mState.get();
+        if (state != State.NOT_STARTED) {
+            throw new IllegalStateException(
+                    "Request is already started. State is: " + mState.get());

[pauljensen, 2015/12/08 19:49:53]

I think you might want to change mState.get() to "state" here?

[Charles, 2015/12/11 16:45:39]

Done.

+        }
+    }
+
+    @Override
+    public void addHeader(String header, String value) {
+        checkNotStarted();
+        if (!isValidHeaderName(header) || value.contains("\r\n")) {
+            throw new IllegalArgumentException("Invalid header " + header + "=" + value);
+        }
+        CaseInsensitiveString string = new CaseInsensitiveString(header);
+        if (mRequestHeaders.containsKey(string)) {
+            mRequestHeaders.remove(string);
+        }
+        mRequestHeaders.put(string, value);
+    }
+
+    private boolean isValidHeaderName(String header) {
+        for (int i = 0; i < header.length(); i++) {
+            char c = header.charAt(i);
+            switch (c) {
+                case '(':
+                case ')':
+                case '<':
+                case '>':
+                case '@':
+                case ',':
+                case ';':
+                case ':':
+                case '\\':
+                case '\'':
+                case '/':
+                case '[':
+                case ']':
+                case '?':
+                case '=':
+                case '{':
+                case '}':
+                    return false;
+                default: {
+                    if (Character.isISOControl(c) || Character.isWhitespace(c)) {
+                        return false;
+                    }
+                }
+            }
+        }
+        return true;
+    }
+
+    @Override
+    public void setUploadDataProvider(UploadDataProvider uploadDataProvider, Executor executor) {
+        if (uploadDataProvider == null) {
+            throw new NullPointerException("Invalid UploadDataProvider.");
+        }
+        if (!mRequestHeaders.containsKey(new CaseInsensitiveString("Content-Type"))) {
+            throw new IllegalArgumentException(
+                    "Requests with upload data must have a Content-Type.");
+        }
+        checkNotStarted();
+        if (mInitialMethod == null) {
+            mInitialMethod = "POST";
+        }
+        this.mUploadDataProvider = uploadDataProvider;
+        this.mUploadExecutor = executor;
+    }
+
+    private final class OutputStreamDataSink implements UploadDataSink {
+        private final Executor mUserExecutor;
+        private final Executor mExecutor;
+        private final HttpURLConnection mUrlConnection;
+        private WritableByteChannel mOutputChannel;
+        private final UploadDataProvider mUploadProvider;
+        private ByteBuffer mBuffer;
+        private long mTotalBytes;
+        private long mWrittenBytes = 0;

[pauljensen, 2015/12/08 19:49:53]

these variables could use some comments, esp the last two

[Charles, 2015/12/11 16:45:39]

Done.

+
+        private OutputStreamDataSink(final Executor userExecutor, Executor executor,
+                HttpURLConnection urlConnection, UploadDataProvider provider) {
+            this.mUserExecutor = new Executor() {
+                @Override
+                public void execute(Runnable runnable) {
+                    try {
+                        userExecutor.execute(runnable);
+                    } catch (RejectedExecutionException e) {
+                        enterUploadErrorState(e);
+                    }
+                }
+            };
+            this.mExecutor = executor;
+            this.mUrlConnection = urlConnection;
+            this.mUploadProvider = provider;
+        }
+
+        @Override
+        public void onReadSucceeded(final boolean finalChunk) {
+            logv("UploadReadSucceeded");

[pauljensen, 2015/12/08 19:49:53]

is this still necessary? ditto for all the following ones

[Charles, 2015/12/11 16:45:39]

Done.

+            mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    mBuffer.flip();
+                    while (mBuffer.hasRemaining()) {
+                        mWrittenBytes += mOutputChannel.write(mBuffer);
+                    }
+                    if (mWrittenBytes < mTotalBytes || (mTotalBytes == -1 && !finalChunk)) {
+                        mBuffer.clear();
+                        mUserExecutor.execute(uploadErrorSetting(new CheckedRunnable() {
+                            @Override
+                            public void run() throws Exception {
+                                mUploadProvider.read(OutputStreamDataSink.this, mBuffer);
+                            }
+                        }));
+                    } else if (mTotalBytes == -1 && finalChunk) {
+                        finish();
+                    } else if (mTotalBytes == mWrittenBytes) {
+                        finish();
+                    } else {
+                        throw new IllegalStateException("Wrote more bytes than were available");
+                    }
+                }
+            }));
+        }
+
+        @Override
+        public void onReadError(Exception exception) {
+            enterUploadErrorState(exception);
+        }
+
+        @Override
+        public void onRewindSucceeded() {
+            logv("UploadRewindSucceeded");
+            startRead();
+        }
+
+        private void startRead() {
+            mUserExecutor.execute(uploadErrorSetting(new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    if (mOutputChannel == null) {
+                        mAdditionalStatusDetails = Status.CONNECTING;
+                        mUrlConnection.connect();
+                        mAdditionalStatusDetails = Status.SENDING_REQUEST;
+                        mOutputChannel = Channels.newChannel(mUrlConnection.getOutputStream());
+                    }
+                    mUploadProvider.read(OutputStreamDataSink.this, mBuffer);
+                }
+            }));
+        }
+
+        @Override
+        public void onRewindError(Exception exception) {
+            enterUploadErrorState(exception);
+        }
+
+        private void finish() throws IOException {
+            if (mOutputChannel != null) {
+                mOutputChannel.close();
+            }
+            mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE;
+            fireGetHeaders();
+        }
+
+        private void start(final boolean firstTime) {
+            mUserExecutor.execute(uploadErrorSetting(new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    mTotalBytes = mUploadProvider.getLength();
+                    if (mTotalBytes == 0) {
+                        finish();
+                    } else {
+                        // If we know how much data we have to upload, and it's small, we can save
+                        // memory by allocating a reasonably sized buffer to read into.
+                        if (mTotalBytes > 0 && mTotalBytes < DEFAULT_UPLOAD_BUFFER_SIZE) {
+                            mBuffer = ByteBuffer.allocateDirect((int) mTotalBytes + 1);
+                        } else {
+                            mBuffer = ByteBuffer.allocateDirect(DEFAULT_UPLOAD_BUFFER_SIZE);
+                        }
+
+                        if (mTotalBytes > 0 && mTotalBytes <= Integer.MAX_VALUE) {
+                            mUrlConnection.setFixedLengthStreamingMode((int) mTotalBytes);
+                        } else if (mTotalBytes > Integer.MAX_VALUE && Build.VERSION.SDK_INT >= 19) {
+                            mUrlConnection.setFixedLengthStreamingMode(mTotalBytes);
+                        } else {
+                            // If we know the length, but we're running pre-kitkat and it's larger
+                            // than an int can hold, we have to use chunked - otherwise we'll end up
+                            // buffering the whole response in memory.
+                            mUrlConnection.setChunkedStreamingMode(DEFAULT_CHUNK_LENGTH);
+                        }
+                        if (firstTime) {
+                            startRead();
+                        } else {
+                            mUploadProvider.rewind(OutputStreamDataSink.this);
+                        }
+                    }
+                }
+            }));
+        }
+    }
+
+    @Override
+    public void start() {
+        mAdditionalStatusDetails = Status.CONNECTING;
+        transitionStates(State.NOT_STARTED, State.STARTED);
+        fireOpenConnection();
+    }
+
+    private void fireOnResponseStarted() {

[pauljensen, 2015/12/08 19:49:53]

how come we have fireOnResponseStarted() but no other ones (e.g.
fireOnSucceeded)?

[Charles, 2015/12/11 16:45:40]

Done.

+        logv("FireOnResponseStarted");
+        mCallbackAsync.onResponseStarted(mUrlResponseInfo);
+    }
+
+    private void enterErrorState(State previousState, final UrlRequestException error) {
+        if (mState.compareAndSet(previousState, State.ERROR)) {
+            fireDisconnect();
+            mCallbackAsync.onFailed(mUrlResponseInfo, error);
+        }
+    }
+
+    /** Ends the reqeust with an error, caused by an exception thrown from user code. */
+    private void enterUserErrorState(State previousState, final Throwable error) {
+        enterErrorState(previousState, new UrlRequestException("User Error", error));
+    }
+
+    /** Ends the requst with an error, caused by an exception thrown from user code. */
+    private void enterUploadErrorState(final Throwable error) {
+        enterErrorState(State.STARTED,
+                new UrlRequestException("Exception received from UploadDataProvider", error));
+    }
+
+    private void enterCronetErrorState(State previousState, final Throwable error) {
+        // TODO(clm) mapping from Java exception (UnknownHostException, for example) to net error
+        // code goes here.
+        enterErrorState(previousState, new UrlRequestException("System error", error));
+    }
+
+    /**
+     * Atomically swaps from the expected state to a new state. If the swap fails, and it's not
+     * due to an earlier error or cancellation, throws an exception.
+     */
+    private void transitionStates(State expected, State newState) {
+        if (!mState.compareAndSet(expected, newState)) {
+            State state = mState.get();
+            if (!(state == State.CANCELLED || state == State.ERROR)) {
+                throw new IllegalStateException(
+                        "Invalid state transition - expected " + expected + " but was " + state);
+            }
+        }
+    }
+
+    @Override
+    public void followRedirect() {
+        logv("FollowRedirect");
+        transitionStates(State.REDIRECTED, State.STARTED);
+        mCurrentUrl = mPendingRedirectUrl;
+        mPendingRedirectUrl = null;
+        fireOpenConnection();
+    }
+
+    private void fireGetHeaders() {
+        logv("FireGetHeaders");
+        mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+            @Override
+            public void run() throws Exception {
+                HttpURLConnection connection = mCurrentUrlConnection.get();
+                if (connection == null) {
+                    return; // We've been cancelled
+                }
+                Map<String, List<String>> headerFields = connection.getHeaderFields();
+
+                String selectedTransport = "http/1.1";
+                if (headerFields.containsKey(X_ANDROID_SELECTED_TRANSPORT)) {
+                    List<String> transports = headerFields.get(X_ANDROID_SELECTED_TRANSPORT);
+                    if (!transports.isEmpty()) {
+                        selectedTransport = transports.get(0);
+                    }
+                }
+
+                final List<Map.Entry<String, String>> headerList = new ArrayList<>();
+                for (Map.Entry<String, List<String>> entry : headerFields.entrySet()) {
+                    // The null header is the status code and message
+                    if (entry.getKey() == null) {
+                        continue;
+                    }
+                    // Android adds synthetic headers - let's not deviate from native cronet
+                    for (String value : entry.getValue()) {
+                        if (!entry.getKey().startsWith("X-Android")) {

[pauljensen, 2015/12/08 19:49:53]

I think the "X-Android" prefix may have changed

[Charles, 2015/12/11 16:45:39]

On my test device, the synthetic headers are still present, and still have
X-Android in them.

[pauljensen, 2015/12/14 20:02:05]

I understand it was this way, but AFAIK it's not anymore according to the
change-log I linked to.

[Charles, 2015/12/15 22:32:35]

I'm not sure I follow - it doesn't matter what changes Android makes, the
devices users actually have in their hands are still running that old code. We
have to handle that behavior.

+                            headerList.add(new SimpleEntry<>(entry.getKey(), value));

[pauljensen, 2015/12/08 19:49:53]

we could tally up the size of entry.getKey() and |value| here and put the sum
into mUrlResponseInfo.setReceivedBytesCount (plus maybe 2 for each header (one
for the ':' and one for the '\n'))

[Charles, 2015/12/11 16:45:39]

We could, but I feel a little dishonest doing it, since the platform is free to
add its own headers that we don't know about.

[pauljensen, 2015/12/14 20:02:05]

How is it dishonest?  The received bytes count is just used to so some really
simplistic data-usage accounting by embedders AFAIK.

[Charles, 2015/12/15 22:32:35]

The simplistic data usage accounting is used to make decisions about what
features cost users what amount of money - I don't want to return information
that's not correct.

[pauljensen, 2015/12/17 19:59:49]

Returning zero is less information than returning something non-zero.  This
value is documented to be a lower bound.  A higher lower bound is better than a
lower one.

[Charles, 2015/12/18 23:37:51]

Alas, we don't get either - since I don't know if the response was gzipped or
not, I can't say that the body size is a lower bound.

+                        }
+                    }
+                }
+                int responseCode = connection.getResponseCode();
+                // Important to copy the list here, because although we never concurrently modify
+                // the list ourselves, user code might iterate over it while we're redirecting, and
+                // that would throw ConcurrentModificationException.
+                mUrlResponseInfo = new UrlResponseInfo(new ArrayList<>(mUrlChain), responseCode,
+                        connection.getResponseMessage(), Collections.unmodifiableList(headerList),
+                        false, selectedTransport, "");
+                // TODO(clm) actual redirect handling? post -> get and whatnot?
+                if (responseCode >= 300 && responseCode < 400) {
+                    fireRedirectReceived(headerFields);
+                } else if (responseCode >= 400) {
+                    mResponseChannel = InputStreamChannel.wrap(connection.getErrorStream());
+                    fireOnResponseStarted();
+                } else {
+                    mResponseChannel = InputStreamChannel.wrap(connection.getInputStream());
+                    fireOnResponseStarted();
+                }
+            }
+        }));
+    }
+
+    private void fireRedirectReceived(Map<String, List<String>> headerFields) {
+        logv("FireRedirectReceived");
+        if (mState.compareAndSet(State.STARTED, State.REDIRECTED)) {
+            mPendingRedirectUrl = headerFields.get("location").get(0);
+            mPendingRedirectUrl = URI.create(mCurrentUrl).resolve(mPendingRedirectUrl).toString();
+            mCallbackAsync.onRedirectReceived(mUrlResponseInfo, mPendingRedirectUrl);
+        }
+    }
+
+    private static final class CaseInsensitiveString {
+        private int mHashCode;
+        private final String mString;
+
+        private CaseInsensitiveString(String string) {
+            if (string == null) {
+                throw new NullPointerException();
+            }
+            mString = string;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            return o instanceof CaseInsensitiveString
+                    && mString.equalsIgnoreCase(((CaseInsensitiveString) o).mString);
+        }
+
+        @Override
+        public int hashCode() {
+            if (mHashCode == 0) {
+                mHashCode = mString.toLowerCase(Locale.US).hashCode();
+            }
+            return mHashCode;
+        }
+
+        String getString() {
+            return mString;
+        }
+    }
+
+    private void fireOpenConnection() {
+        logv("FireOpenConnection");
+        mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+            @Override
+            public void run() throws Exception {
+                // If we're cancelled, then our old connection will be disconnected for us and
+                // we shouldn't open a new one.
+                if (mState.get() == State.CANCELLED) {
+                    return;
+                }
+
+                final URL url = new URL(mCurrentUrl);
+                HttpURLConnection newConnection = (HttpURLConnection) url.openConnection();
+                HttpURLConnection oldConnection = mCurrentUrlConnection.getAndSet(newConnection);
+                if (oldConnection != null) {
+                    oldConnection.disconnect();
+                    // It could be null because we're cancelled, or it could be null because this

[pauljensen, 2015/12/08 19:49:53]

I find this comment a little confusing.  What is "It" that could be null?  I'm
guessing oldConnection, but then why are we discussing the case of null
oldConnection in a if statement with clause "oldConnection != null"?

[Charles, 2015/12/11 16:45:39]

Done.

+                    // is not a redirect. We haven't connected the new connection yet, so no need to
+                    // disconnect it.
+                    if (mState.get() == State.CANCELLED) {
+                        return;
+                    }
+                }
+                mUrlChain.add(mCurrentUrl);
+                newConnection.setInstanceFollowRedirects(false);
+                CaseInsensitiveString userAgent = new CaseInsensitiveString("User-Agent");
+                if (!mRequestHeaders.containsKey(userAgent)) {
+                    mRequestHeaders.put(userAgent, mUserAgent);
+                }
+                for (Map.Entry<CaseInsensitiveString, String> entry : mRequestHeaders.entrySet()) {
+                    newConnection.setRequestProperty(entry.getKey().getString(), entry.getValue());
+                }
+                if (mInitialMethod == null) {
+                    mInitialMethod = "GET";
+                }
+                newConnection.setRequestMethod(mInitialMethod);
+                if (mUploadDataProvider != null) {
+                    OutputStreamDataSink dataSink = new OutputStreamDataSink(
+                            mUploadExecutor, mExecutor, newConnection, mUploadDataProvider);
+                    dataSink.start(mUrlChain.size() == 1);
+                } else {
+                    mAdditionalStatusDetails = Status.CONNECTING;
+                    newConnection.connect();
+                    mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE;
+                    fireGetHeaders();
+                }
+            }
+        }));
+    }
+
+    @Override
+    public void read(final ByteBuffer buffer) {
+        Preconditions.checkDirect(buffer);
+        Preconditions.checkHasRemaining(buffer);

[pauljensen, 2015/12/08 19:49:53]

this will fail inadvertently in certain cases.  this call expects distance
between position and capacity.  position may be equal to limit, in which case
checkHasRemaining will fail inadvertently.

[Charles, 2015/12/11 16:45:39]

Good catch. 

+        transitionStates(State.AWAITING_READ, State.READING);
+        mExecutor.execute(errorSetting(State.READING, new CheckedRunnable() {
+            @Override
+            public void run() throws IOException {
+                int oldPosition = buffer.position();
+                int read = mResponseChannel.read(buffer);
+                buffer.limit(buffer.position());
+                buffer.position(oldPosition);
+                processReadResult(read, buffer);
+            }
+        }));
+    }
+
+    private Runnable errorSetting(final State expectedState, final CheckedRunnable delegate) {
+        return new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    delegate.run();
+                } catch (Throwable t) {
+                    enterCronetErrorState(expectedState, t);
+                }
+            }
+        };
+    }
+
+    private Runnable userErrorSetting(final State expectedState, final CheckedRunnable delegate) {
+        return new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    delegate.run();
+                } catch (Throwable t) {
+                    enterUserErrorState(expectedState, t);
+                }
+            }
+        };
+    }
+
+    private Runnable uploadErrorSetting(final CheckedRunnable delegate) {
+        return new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    delegate.run();
+                } catch (Throwable t) {
+                    enterUploadErrorState(t);
+                }
+            }
+        };
+    }
+
+    private interface CheckedRunnable { void run() throws Exception; }
+
+    @Override
+    public void readNew(final ByteBuffer buffer) {
+        Preconditions.checkDirect(buffer);
+        Preconditions.checkHasRemaining(buffer);
+        transitionStates(State.AWAITING_READ, State.READING);
+        mExecutor.execute(errorSetting(State.READING, new CheckedRunnable() {
+            @Override
+            public void run() throws Exception {
+                int read = mResponseChannel.read(buffer);
+                processReadResult(read, buffer);
+            }
+        }));
+    }
+
+    private void processReadResult(int read, final ByteBuffer buffer) throws IOException {
+        if (read != -1) {

[pauljensen, 2015/12/08 19:49:52]

seems like we could call mUrlResponseInfo.setReceivedBytesCount(read +
mUrlResponseInfo.getReceivedBytesCount())

[Charles, 2015/12/11 16:45:39]

Unfortunately, gzip is transparent - we don't know the real count.

+            mCallbackAsync.onReadCompleted(mUrlResponseInfo, buffer);
+        } else {
+            mResponseChannel.close();
+            if (mState.compareAndSet(State.READING, State.COMPLETE)) {
+                fireDisconnect();
+                mCallbackAsync.onSucceeded(mUrlResponseInfo);
+            }
+        }
+    }
+
+    private void fireDisconnect() {
+        final HttpURLConnection connection = mCurrentUrlConnection.getAndSet(null);
+        mExecutor.execute(new Runnable() {
+            @Override
+            public void run() {
+                if (connection != null) {
+                    connection.disconnect();
+                }
+            }
+        });
+    }
+
+    @Override
+    public void cancel() {
+        State oldState = mState.getAndSet(State.CANCELLED);
+        switch (oldState) {
+            // We've just scheduled some user code to run. When they perform their next operation,
+            // they'll observe it and fail. However, if user code is cancelling in response to one
+            // of these callbacks, we'll never actually cancel!
+            // TODO(clm) figure out if it's possible to avoid concurrency in user callbacks.
+            case REDIRECTED:
+            case AWAITING_READ:
+
+            // User code is waiting on us - cancel away!
+            case STARTED:
+            case READING:
+                fireDisconnect();
+                mCallbackAsync.onCanceled(mUrlResponseInfo);
+                break;
+            // The rest are all termination cases - we're too late to cancel.
+            case ERROR:
+            case COMPLETE:
+            case CANCELLED:
+                break;
+        }
+    }
+
+    @Override
+    public boolean isDone() {
+        State state = mState.get();
+        return state == State.COMPLETE | state == State.ERROR | state == State.CANCELLED;
+    }
+
+    @Override
+    public void disableCache() {
+        // We have no cache
+    }
+
+    @Override
+    public void getStatus(StatusListener listener) {
+        State state = mState.get();
+        int extraStatus = this.mAdditionalStatusDetails;
+
+        @Status.StatusValues final int status;
+        switch (state) {
+            case ERROR:
+            case COMPLETE:
+            case CANCELLED:
+            case NOT_STARTED:
+                status = Status.INVALID;
+                break;
+            case STARTED:
+                status = extraStatus;
+                break;
+            case REDIRECTED:
+            case AWAITING_READ:
+                status = Status.IDLE;
+                break;
+            case READING:
+                status = Status.READING_RESPONSE;
+                break;
+            default:
+                throw new IllegalStateException("Switch is exhaustive: " + state);
+        }
+
+        mCallbackAsync.sendStatus(listener, status);
+    }
+
+    private static void logv(String log) {
+        if (Log.isLoggable(TAG, Log.VERBOSE)) {
+            Log.v(TAG, log);
+        }
+    }
+
+    /** This wrapper ensures that callbacks are always called on the correct executor */
+    private final class AsyncUrlRequestCallback {
+        private final UrlRequest.Callback mCallback;
+        private final Executor mUserExecutor;
+
+        private AsyncUrlRequestCallback(Callback callback, final Executor userExecutor) {
+            this.mCallback = callback;
+            this.mUserExecutor = userExecutor;
+        }
+
+        void sendStatus(final StatusListener listener, final int status) {
+            mUserExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    listener.onStatus(status);
+                }
+            });
+        }
+
+        void execute(State currentState, CheckedRunnable runnable) {
+            try {
+                mUserExecutor.execute(userErrorSetting(currentState, runnable));
+            } catch (RejectedExecutionException e) {
+                enterUserErrorState(currentState, e);
+            }
+        }
+
+        void onRedirectReceived(final UrlResponseInfo info, final String newLocationUrl) {
+            execute(State.REDIRECTED, new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    mCallback.onRedirectReceived(JavaUrlRequest.this, info, newLocationUrl);
+                }
+            });
+        }
+
+        void onResponseStarted(UrlResponseInfo info) {
+            execute(State.AWAITING_READ, new CheckedRunnable() {
+                @Override
+                public void run() {
+                    if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) {
+                        mCallback.onResponseStarted(JavaUrlRequest.this, mUrlResponseInfo);
+                    }
+                }
+            });
+        }
+
+        void onReadCompleted(final UrlResponseInfo info, final ByteBuffer byteBuffer) {
+            execute(State.AWAITING_READ, new CheckedRunnable() {
+                @Override
+                public void run() {
+                    if (mState.compareAndSet(State.READING, State.AWAITING_READ)) {
+                        mCallback.onReadCompleted(JavaUrlRequest.this, info, byteBuffer);
+                    }
+                }
+            });
+        }
+
+        void onCanceled(final UrlResponseInfo info) {
+            closeQuietly(mResponseChannel);
+            mUserExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    try {
+                        mCallback.onCanceled(JavaUrlRequest.this, info);
+                    } catch (Exception exception) {
+                        Log.e(TAG, "Exception in onCanceled method", exception);
+                    }
+                }
+            });
+        }
+
+        void onSucceeded(final UrlResponseInfo info) {
+            mUserExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    try {
+                        mCallback.onSucceeded(JavaUrlRequest.this, info);
+                    } catch (Exception exception) {
+                        Log.e(TAG, "Exception in onSucceeded method", exception);
+                    }
+                }
+            });
+        }
+
+        void onFailed(final UrlResponseInfo urlResponseInfo, final UrlRequestException e) {
+            closeQuietly(mResponseChannel);
+            mUserExecutor.execute(new Runnable() {
+                @Override
+                public void run() {
+                    try {
+                        mCallback.onFailed(JavaUrlRequest.this, urlResponseInfo, e);
+                    } catch (Exception exception) {
+                        Log.e(TAG, "Exception in onFailed method", exception);
+                    }
+                }
+            });
+        }
+    }
+
+    private static void closeQuietly(Closeable closeable) {
+        if (closeable == null) {
+            return;
+        }
+        try {
+            closeable.close();
+        } catch (IOException e) {
+            e.printStackTrace();
+        }
+    }
+}