Initial implementation of Cronet Async API.

components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.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;
 
+import android.util.Log;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+
+import java.nio.ByteBuffer;
+import java.util.AbstractMap;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+
 /**
- * UrlRequest using Chromium HTTP stack implementation.
+ * UrlRequest using Chromium HTTP stack implementation. Could be accessed from
+ * any thread on Executor. Cancel can be done from any thread.
+ * All @CallByNative methods are called on native network thread
+ * and post tasks with listener calls onto Executor. Upon return from listener
+ * callback native request adapter is called on executive thread and posts
+ * native tasks to native network thread. Because Cancel could be called from
+ * any thread it is protected by mUrlRequestAdapterLock.

[mmenke, 2014/11/06 16:08:27]

I think this comment can be cleaned up.  I can provide suggestions when I have
time, if you want.

[mef, 2014/11/06 17:07:47]

I'll appreciate that.

  */
-public class CronetUrlRequest implements UrlRequest {
+@JNINamespace("cronet")
+final class CronetUrlRequest implements UrlRequest {
+    /** Native adapter object, owned by UrlRequest. */

[mmenke, 2014/11/06 16:08:27]

Should this use the "/**"?  I assume the others don't because Javadoc uses it.

[mef, 2014/11/06 22:51:46]

Done.

+    private long mUrlRequestAdapter;
+    private boolean mStarted = false;
+    private boolean mCanceled = false;
+    private boolean mInOnDataReceived = false;
+    private boolean mDestroyRequestAdapterAfterOnDataReceived = false;
+    /*
+     * Synchronize access to mUrlRequestAdapter, mStarted, mCanceled and
+     * mDestroyAfterReading.
+     */
+    private final Object mUrlRequestAdapterLock = new Object();
+    private final CronetUrlRequestContext mRequestContext;
+    private final Executor mExecutor;
+    /*

[mmenke, 2014/11/06 16:08:27]

Suggest a blank line before each of these multi-line comments, for legibility.

[mef, 2014/11/06 22:51:47]

Done.

+     * Url chain contans the URL currently being requested, and
+     * all URLs previously requested.  New URLs are only added after it is
+     * decided a redirect will be followed.
+     */
+    private final List<String> mUrlChain = new ArrayList<String>();
+
+    private final UrlRequestListener mListener;
+    private final String mInitialUrl;
+    private final int mPriority;
+    private String mInitialMethod;
+    private final HeadersList mRequestHeaders = new HeadersList();
+
+    private NativeResponseInfo mResponseInfo;
+
+    /*
+     * Listener callback is repeatedly called when data is received, so it is
+     * cached as member variable.
+     */
+    private OnDataReceivedRunnable mOnDataReceivedTask;
+
+    static final class HeaderEntry extends
+            AbstractMap.SimpleEntry<String, String> {
+        public HeaderEntry(String name, String value) {
+            super(name, value);
+        }
+    }
+
+    static final class HeadersList extends ArrayList<HeaderEntry> {
+    }
+
+    final class OnDataReceivedRunnable implements Runnable {
+        ByteBuffer mByteBuffer;
+
+        public void run() {
+            if (isCanceled()) {
+                return;
+            }
+            try {
+                synchronized (mUrlRequestAdapterLock) {
+                    if (mUrlRequestAdapter == 0) {
+                        return;
+                    }
+                    mInOnDataReceived = true;

[mmenke, 2014/11/06 16:08:27]

Should explicitly mention why this is needed.

[mef, 2014/11/06 22:51:46]

Done.

+                }
+                mListener.onDataReceived(CronetUrlRequest.this,
+                        mResponseInfo, mByteBuffer);
+                mByteBuffer = null;
+                synchronized (mUrlRequestAdapterLock) {
+                    mInOnDataReceived = false;
+                    if (mDestroyRequestAdapterAfterOnDataReceived) {
+                        destroyRequestAdapter();
+                    }
+                    if (isCanceled()) {

[mmenke, 2014/11/06 16:08:27]

Can just merge these and get rid of mDestroyRequestAdapterAfterOnDataReceived:

if (isCanceled()) {
  destroyRequestAdapter();
  return;
}

[mef, 2014/11/06 22:51:46]

Done.

+                        return;
+                    }
+                    nativeReceiveData(mUrlRequestAdapter);
+                }
+            } catch (Exception e) {
+                synchronized (mUrlRequestAdapterLock) {
+                    mInOnDataReceived = false;
+                    if (mDestroyRequestAdapterAfterOnDataReceived) {
+                        destroyRequestAdapter();
+                    }
+                }
+                onCalledByNativeException(e);
+            }
+        }
+    }
+
+    static final class NativeResponseInfo implements ResponseInfo {
+        private final String[] mResponseInfoUrlChain;
+        private final int mHttpStatusCode;
+        private final HeadersMap mAllHeaders = new HeadersMap();
+        private final boolean mWasCached;
+        private final String mNegotiatedProtocol;
+
+        NativeResponseInfo(String[] urlChain, int httpStatusCode,
+                boolean wasCached, String negotiatedProtocol) {
+            mResponseInfoUrlChain = urlChain;
+            mHttpStatusCode = httpStatusCode;
+            mWasCached = wasCached;
+            mNegotiatedProtocol = negotiatedProtocol;
+        }
+
+        @Override
+        public String getUrl() {
+            return mResponseInfoUrlChain[mResponseInfoUrlChain.length - 1];
+        }
+
+        @Override
+        public String[] getUrlChain() {
+            return mResponseInfoUrlChain;
+        }
+
+        @Override
+        public int getHttpStatusCode() {
+            return mHttpStatusCode;
+        }
+
+        @Override
+        public Map<String, List<String>> getAllHeaders() {
+            return mAllHeaders;
+        }
+
+        @Override
+        public boolean wasCached() {
+            return mWasCached;
+        }
+
+        @Override
+        public String getNegotiatedProtocol() {
+            return mNegotiatedProtocol;
+        }
+    };
+
+    static final class NativeExtendedResponseInfo implements
+            ExtendedResponseInfo {
+        private final ResponseInfo mResponseInfo;
+        private final long mTotalReceivedBytes;
+
+        NativeExtendedResponseInfo(ResponseInfo responseInfo,
+                                   long totalReceivedBytes) {
+            mResponseInfo = responseInfo;
+            mTotalReceivedBytes = totalReceivedBytes;
+        }
+
+        @Override
+        public ResponseInfo getResponseInfo() {
+            return mResponseInfo;
+        }
+
+        @Override
+        public long getTotalReceivedBytes() {
+            return mTotalReceivedBytes;
+        }
+    };
+
+    CronetUrlRequest(CronetUrlRequestContext requestContext,
+            long urlRequestContextAdapter,
+            String url,
+            int priority,
+            UrlRequestListener listener,
+            Executor executor) {
+        if (url == null) {
+            throw new NullPointerException("URL is required");
+        }
+        if (listener == null) {
+            throw new NullPointerException("Listener is required");
+        }
+        if (executor == null) {
+            throw new NullPointerException("Executor is required");
+        }
+
+        mRequestContext = requestContext;
+        mInitialUrl = url;
+        mUrlChain.add(url);
+        mPriority = convertRequestPriority(priority);
+        mListener = listener;
+        mExecutor = executor;
+    }
+
     @Override
     public void setHttpMethod(String method) {
-
+        checkNotStarted();
+        if (method == null) {
+            throw new NullPointerException("Method is required.");
+        }
+        mInitialMethod = method;
     }
 
     @Override
     public void addHeader(String header, String value) {
-
-    }
-
-    @Override
-    public void start(UrlRequestListener listener) {
-
+        checkNotStarted();
+        if (header == null) {
+            throw new NullPointerException("Invalid header name.");
+        }
+        if (value == null) {
+            throw new NullPointerException("Invalid header value.");
+        }
+        mRequestHeaders.add(new HeaderEntry(header, value));
+    }
+
+    @Override
+    public void start() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter != 0) {

[mmenke, 2014/11/06 16:08:27]

Should check mStarted instead (It prevents restarting after cancellation or
completion).  Or better, just call checkNotStarted().

[mef, 2014/11/06 22:51:46]

Done.

+                throw new IllegalStateException("Request is already started.");
+            }
+            mUrlRequestAdapter = nativeCreateRequestAdapter(
+                    mRequestContext.getUrlRequestContextAdapter(),
+                    mInitialUrl,
+                    mPriority);
+            if (mInitialMethod != null) {
+                if (!nativeSetHttpMethod(mUrlRequestAdapter, mInitialMethod)) {
+                    nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                    mUrlRequestAdapter = 0;

[mmenke, 2014/11/06 16:08:27]

Replace these two lines with destroyRequestAdapter()?  And same below.

Alternatively, could do a try/catch, and destroy in the catch clause (And
re-throw the exception, though you'll need to make sure you're only catching
silently an exception type that doesn't need an explicit throws declaration).

[mef, 2014/11/06 22:51:46]

Done.

+                    throw new IllegalArgumentException("Invalid http method "
+                            + mInitialMethod);
+                }
+            }
+            for (HeaderEntry header : mRequestHeaders) {
+                if (!nativeAddHeader(mUrlRequestAdapter, header.getKey(),
+                        header.getValue())) {
+                    nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                    mUrlRequestAdapter = 0;
+                    throw new IllegalArgumentException("Invalid header "
+                            + header.getKey() + "=" + header.getValue());
+                }
+            }
+            mStarted = true;
+            nativeStart(mUrlRequestAdapter);
+            mRequestContext.onRequestStarted(this);
+        }
     }
 
     @Override
     public void cancel() {
-
+        synchronized (mUrlRequestAdapterLock) {
+            if (mCanceled) {
+                return;
+            }
+            mCanceled = true;
+            // During call into listener OnDataReceived adapter cannot be
+            // destroyed as it owns the byte buffer.
+            if (mInOnDataReceived) {
+                mDestroyRequestAdapterAfterOnDataReceived = true;
+            } else {
+                destroyRequestAdapter();
+            }
+        }
     }
 
     @Override
     public boolean isCanceled() {
-        return false;
+        synchronized (mUrlRequestAdapterLock) {
+            return mCanceled;
+        }
     }
 
     @Override
     public void pause() {
-
-    }
-
-    @Override
+        throw new UnsupportedOperationException("Not implemented yet");
+    }
+
+    @Override
     public boolean isPaused() {
         return false;
     }
 
     @Override
     public void resume() {
-
+        throw new UnsupportedOperationException("Not implemented yet");
+    }
+
+    /**
+     * Post task to application Executor. Used for Listener callbacks
+     * and other tasks that should not be executed on network thread.
+     */
+    private void postTaskToExecutor(Runnable task) {
+        mExecutor.execute(task);
+    }
+
+    private static int convertRequestPriority(int priority) {
+        switch (priority) {
+            case REQUEST_PRIORITY_IDLE:
+                return RequestPriority.IDLE;
+            case REQUEST_PRIORITY_LOWEST:
+                return RequestPriority.LOWEST;
+            case REQUEST_PRIORITY_LOW:
+                return RequestPriority.LOW;
+            case REQUEST_PRIORITY_MEDIUM:
+                return RequestPriority.MEDIUM;
+            case REQUEST_PRIORITY_HIGHEST:
+                return RequestPriority.HIGHEST;
+            default:
+                return RequestPriority.MEDIUM;
+        }
+    }
+
+    private NativeResponseInfo prepareResponseInfo(int httpStatusCode) {

[mmenke, 2014/11/06 16:08:27]

Should either rename network thread methods to make it clear where they're run,
or comment about it.

[mef, 2014/11/06 22:51:46]

Done.

+        long urlRequestAdapter;
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter == 0) {
+                return null;
+            }
+            urlRequestAdapter = mUrlRequestAdapter;
+        }
+        NativeResponseInfo responseInfo = new NativeResponseInfo(
+                mUrlChain.toArray(new String[mUrlChain.size()]),
+                httpStatusCode,
+                nativeGetWasCached(urlRequestAdapter),
+                nativeGetNegotiatedProtocol(urlRequestAdapter));

[mmenke, 2014/11/06 16:08:27]

Should mention why all these calls into the adapter are safe, even if
mUrlRequestAdapter may have been 0ed out.

[mef, 2014/11/06 22:51:46]

Done.

+        nativePopulateResponseHeaders(urlRequestAdapter,
+                                      responseInfo.mAllHeaders);
+        return responseInfo;
+    }
+
+    private void checkNotStarted() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mStarted) {
+                throw new IllegalStateException("Request is already started.");
+            }

[mmenke, 2014/11/06 16:08:26]

Should we check if we're cancelled, too?  Seems like being cancelled before
start could cause Bad Things(tm)

[mef, 2014/11/06 22:51:46]

Done.

+        }
+    }
+
+    private void destroyRequestAdapter() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter == 0) {
+                return;
+            }
+            nativeDestroyRequestAdapter(mUrlRequestAdapter);
+            mRequestContext.onRequestDestroyed(this);
+            mUrlRequestAdapter = 0;
+        }
+    }
+
+    private void appendHeaderToMap(HeadersMap headersMap,

[mmenke, 2014/11/06 16:08:27]

static...Actually, I suggest just inlining this, think it's clearer that way,
rather than having it a couple hundred lines away from the one method that
invokes it.

[mef, 2014/11/06 22:51:46]

Done.

+            String name, String value) {
+        if (!headersMap.containsKey(name)) {
+            headersMap.put(name, new ArrayList<String>());
+        }
+        headersMap.get(name).add(value);
+    }
+
+    /**
+     * If @CalledByNative method throws an exception, request gets canceled
+     * and exception could be retrieved from request using getException().

[mmenke, 2014/11/06 16:08:27]

"Only called on the Executor".

[mmenke, 2014/11/06 16:08:27]

There is no GetException method.  We actually pass in the exception to the
listener's onFailed method.

[mef, 2014/11/06 22:51:46]

Done.

+     */
+    private void onCalledByNativeException(Exception e) {

[mmenke, 2014/11/06 16:08:27]

If the request has already been cancelled, we should probably do nothing -
should consistently try not to call into mListener after cancllation, I think.

Could even use a synchronized block to prevent double cancellation, though not
sure if that really gets us much.

[mmenke, 2014/11/06 16:08:27]

Both the name and description aren't terribly accurate - these are exceptions
thrown by the listener when called by Runnables that are posted to executors in
CalledByNative methods.

Maybe onListenerException?

[mef, 2014/11/06 22:51:46]

Done.

+        UrlRequestException requestError = new UrlRequestException(
+                "CalledByNative method has thrown an exception", e);
+        Log.e(CronetUrlRequestContext.LOG_TAG,
+                "Exception in CalledByNative method", e);
+        try {
+            cancel();
+            mListener.onFailed(this, mResponseInfo, requestError);
+        } catch (Exception cancelException) {
+            Log.e(CronetUrlRequestContext.LOG_TAG,
+                    "Exception trying to cancel request", cancelException);
+        }
+    }
+
+    ////////////////////////////////////////////////
+    // Private methods called by the native code.
+    ////////////////////////////////////////////////
+
+    /**
+     * Called before following redirects. The redirect will automatically be
+     * followed, unless the request is paused or canceled during this
+     * callback. If the redirect response has a body, it will be ignored.
+     * This will only be called between start and onResponseStarted.
+     *
+     * @param newLocation Location where request is redirected.
+     * @param httpStatusCode from redirect response
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onRedirect(final String newLocation, int httpStatusCode) {
+        final NativeResponseInfo responseInfo =
+                prepareResponseInfo(httpStatusCode);
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                try {
+                    mListener.onRedirect(CronetUrlRequest.this, responseInfo,
+                            newLocation);
+                    synchronized (mUrlRequestAdapterLock) {
+                        if (isCanceled()) {
+                            return;
+                        }
+                        // It is Ok to access mUrlChain not on the network
+                        // thread as the request is waiting to follow redirect.
+                        mUrlChain.add(newLocation);
+                        nativeFollowDeferredRedirect(mUrlRequestAdapter);
+                    }
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        postTaskToExecutor(task);
+    }
+
+    /**
+     * Called when the final set of headers, after all redirects,
+     * is received. Can only be called once for each request.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onResponseStarted(int httpStatusCode) {
+        mResponseInfo = prepareResponseInfo(httpStatusCode);
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                try {
+                    mListener.onResponseStarted(CronetUrlRequest.this,
+                                                mResponseInfo);
+                    synchronized (mUrlRequestAdapterLock) {
+                        if (isCanceled()) {
+                            return;
+                        }
+                        nativeReceiveData(mUrlRequestAdapter);
+                    }
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        postTaskToExecutor(task);
+    }
+
+    /**
+     * Called whenever data is received. The ByteBuffer remains
+     * valid only until listener callback. Or if the callback
+     * pauses the request, it remains valid until the request is resumed.
+     * Cancelling the request also invalidates the buffer.
+     *
+     * @param byteBuffer Received data.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onDataReceived(final ByteBuffer byteBuffer) {
+        if (mOnDataReceivedTask == null) {
+            mOnDataReceivedTask = new OnDataReceivedRunnable();
+        }
+        mOnDataReceivedTask.mByteBuffer = byteBuffer;
+        postTaskToExecutor(mOnDataReceivedTask);
+    }
+
+    /**
+     * Called when request is complete, no callbacks will be called afterwards.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onComplete() {

[mmenke, 2014/11/06 16:08:27]

onSucceeded?

[mef, 2014/11/06 22:51:47]

Done.

+        long totalReceivedBytes;
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter == 0) {
+                return;
+            }
+            totalReceivedBytes =
+                    nativeGetTotalReceivedBytes(mUrlRequestAdapter);
+        }
+
+        final NativeExtendedResponseInfo extendedResponseInfo =
+                new NativeExtendedResponseInfo(mResponseInfo,
+                        totalReceivedBytes);
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                // Destroy adapter first, so request context could be shut down
+                // from the listener.
+                destroyRequestAdapter();

[mmenke, 2014/11/06 16:08:27]

Hrm...Does it make sense to grab the lock, check is cancelled, and then destroy
the adapter?  Feels a little safer, but I'm not sure it lets us may any strong
guarantees.

[mef, 2014/11/06 22:51:46]

Done.

+                try {
+                    mListener.onSucceeded(CronetUrlRequest.this,
+                                          extendedResponseInfo);
+                } catch (Exception e) {
+                    Log.e(CronetUrlRequestContext.LOG_TAG,
+                            "Exception in onComplete method", e);
+                }
+            }
+        };
+        postTaskToExecutor(task);
+    }
+
+    /**
+     * Called when error has occured, no callbacks will be called afterwards.
+     *
+     * @param nativeError native net error code.
+     * @param errorString textual representation of the error code.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onError(final int nativeError, final String errorString) {
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                // Destroy adapter first, so request context could be shut down
+                // from the listener.
+                destroyRequestAdapter();
+                try {
+                    UrlRequestException requestError = new UrlRequestException(
+                            "Exception in CronetUrlRequest: " + errorString,
+                            nativeError);
+                    mListener.onFailed(CronetUrlRequest.this,
+                                       mResponseInfo,
+                                       requestError);
+                } catch (Exception e) {
+                    Log.e(CronetUrlRequestContext.LOG_TAG,
+                            "Exception in onError method", e);
+                }
+            }
+        };
+        postTaskToExecutor(task);
+    }
+
+    /**
+     * Appends header |name| with value |value| to |headersMap|.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onAppendResponseHeader(HeadersMap headersMap,
+            String name, String value) {
+        try {

[mmenke, 2014/11/06 16:08:27]

Is this try block even needed?  This is a weird enough way to fail that if it is
needed, we should have a test for it (The call stack is
C++->Java->C++->Java->throws exception, and it's easy to imagine some sort of
change that ends up on giving us an extra Java call into the listener in this
case).

[mef, 2014/11/06 17:07:47]

Hmm, maybe not.

[mef, 2014/11/06 22:51:47]

Done.

+            appendHeaderToMap(headersMap, name, value);
+        } catch (final Exception e) {
+            onCalledByNativeException(e);
+            Runnable task = new Runnable() {
+                public void run() {
+                    if (isCanceled()) {
+                        return;
+                    }
+                    onCalledByNativeException(e);
+                }
+            };
+            postTaskToExecutor(task);
+        }
+    }
+
+    // Native methods are implemented in cronet_url_request.cc.
+
+    private native long nativeCreateRequestAdapter(
+            long urlRequestContextAdapter, String url, int priority);
+
+    private native boolean nativeAddHeader(long urlRequestAdapter, String name,
+            String value);
+
+    private native boolean nativeSetHttpMethod(long urlRequestAdapter,
+            String method);
+
+    private native void nativeStart(long urlRequestAdapter);
+
+    private native void nativeDestroyRequestAdapter(long urlRequestAdapter);
+
+    private native void nativeFollowDeferredRedirect(long urlRequestAdapter);
+
+    private native void nativeReceiveData(long urlRequestAdapter);
+
+    private native void nativePopulateResponseHeaders(long urlRequestAdapter,
+            HeadersMap headers);
+
+    private native String nativeGetNegotiatedProtocol(long urlRequestAdapter);
+
+    private native boolean nativeGetWasCached(long urlRequestAdapter);
+
+    private native long nativeGetTotalReceivedBytes(long urlRequestAdapter);
+
+    // Explicit class to work around JNI-generator generics confusion.
+    private static class HeadersMap extends HashMap<String, List<String>> {
     }
 }