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.net.URL;
+import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+import java.util.concurrent.atomic.AtomicBoolean;
+
 /**
  * UrlRequest using Chromium HTTP stack implementation.
  */
-public class CronetUrlRequest implements UrlRequest {
+@JNINamespace("cronet")
+final class CronetUrlRequest implements UrlRequest {
+    /** Native adapter object, owned by UrlRequest. */
+    private long mUrlRequestAdapter;
+    private final Object mUrlRequestAdapterLock = new Object();

[mmenke, 2014/10/29 16:20:47]

Do we get anything from using a new object, instead of "synchronized(this)"? 
Just ensures that if the embedder uses synchronized(CronetUrlRequest), Bad
Things won't happen?

[mef, 2014/10/29 19:05:00]

Yeah, chrome's findbugs script complains about synchronized(this).

+    private final CronetUrlRequestContext mRequestContext;
+    private final List<String> mUrlChain = new ArrayList<String>();
+    private final int mPriority;
+    private final UrlRequestListener mListener;
+    private final Executor mExecutor;
+    private final String mInitialUrl;
+    private String mMethod;
+    private final HeadersMap mRequestHeaders = new HeadersMap();
+    private NativeResponseInfo mResponseInfo;
+    private AtomicBoolean mCanceled = new AtomicBoolean(false);

[mmenke, 2014/10/29 16:20:47]

Having both an atomic and a lock seems a bit confusing here - it doesn't break
anything, but you can be in a state where mCanceled is true, but
mUrlRequestAdapter has not yet been NULLed out, and you're accessing
mUrlRequestAdapter on another thread.

My suggestion:  Get rid of cancelled, and just grab the lock to check for
cancellation.

[mef, 2014/10/29 19:05:01]

Done.

+    private OnDataReceivedRunnable mOnDataReceivedTask;
+
+    final class OnDataReceivedRunnable implements Runnable {
+        ByteBuffer mByteBuffer;
+        public void run() {
+            if (isCanceled()) {
+                return;
+            }
+            try {
+                mListener.onDataReceived(CronetUrlRequest.this,
+                        mResponseInfo, mByteBuffer);
+                mByteBuffer = null;
+                synchronized (mUrlRequestAdapterLock) {
+                    nativeReceiveData(mUrlRequestAdapter);
+                }
+            } catch (Exception e) {
+                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;
+        }
+    };
+
+    final class NativeExtendedResponseInfo implements ExtendedResponseInfo {
+        ResponseInfo mResponseInfo;
+        private long mTotalReceivedBytes = 0;

[mmenke, 2014/10/29 16:20:48]

Both these can be private final (And the "= 0" is not needed)

[mef, 2014/10/29 19:05:00]

Done.

+
+        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 (requestContext == null) {
+            throw new NullPointerException("Context is required");
+        }
+        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.");
+        }
+        mMethod = method;
     }
 
     @Override
     public void addHeader(String header, String value) {
-
-    }
-
-    @Override
-    public void start(UrlRequestListener listener) {
-
+        checkNotStarted();
+        if (header == null || value == null) {
+            throw new NullPointerException("Invalid header.");
+        }
+        appendHeaderToMap(mRequestHeaders, header, value);
+    }
+
+    @Override
+    public void start() {
+        synchronized (mUrlRequestAdapterLock) {

[mmenke, 2014/10/29 16:20:48]

Should also check if we've been cancelled?

[mef, 2014/10/29 19:05:01]

Done.

+            if (mUrlRequestAdapter != 0) {
+                throw new IllegalStateException("Request is already started.");
+            }
+            mUrlRequestAdapter = nativeCreateRequestAdapter(
+                    mRequestContext.getUrlRequestContextAdapter(),
+                    mInitialUrl,
+                    mPriority);
+            if (mMethod != null) {
+                nativeSetHttpMethod(mUrlRequestAdapter, mMethod);
+            }
+            for (Map.Entry<String, List<String>> header :
+                    mRequestHeaders.entrySet()) {
+                for (String headerValue : header.getValue()) {
+                    if (!nativeAddHeader(mUrlRequestAdapter, header.getKey(),
+                            headerValue)) {
+                        nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                        mUrlRequestAdapter = 0;
+                        throw new IllegalArgumentException("Invalid header.");

[mmenke, 2014/10/29 16:20:48]

Should include the key/value in the error message.

[mef, 2014/10/29 19:05:00]

Done.

+                    }
+                }
+            }
+            nativeStart(mUrlRequestAdapter);
+            mRequestContext.onRequestStarted(this);
+        }
     }
 
     @Override
     public void cancel() {
-
+        if (mCanceled.compareAndSet(false, true)) {
+            destroyRequestAdapter();
+        }
     }
 
     @Override
     public boolean isCanceled() {
-        return false;
+        return mCanceled.get();
     }
 
     @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 or Looper. Used for Listener callbacks
+     * and other tasks that should not be executed on network thread.
+     */
+    private void postAppTask(Runnable task) {
+        mExecutor.execute(task);
+    }
+
+    private static int convertRequestPriority(int priority) {
+        switch (priority) {
+            case REQUEST_PRIORITY_IDLE:
+                return ChromiumUrlRequestPriority.IDLE;
+            case REQUEST_PRIORITY_LOWEST:
+                return ChromiumUrlRequestPriority.LOWEST;
+            case REQUEST_PRIORITY_LOW:
+                return ChromiumUrlRequestPriority.LOW;
+            case REQUEST_PRIORITY_MEDIUM:
+                return ChromiumUrlRequestPriority.MEDIUM;
+            case REQUEST_PRIORITY_HIGHEST:
+                return ChromiumUrlRequestPriority.HIGHEST;
+            default:
+                return ChromiumUrlRequestPriority.MEDIUM;
+        }
+    }
+
+    private NativeResponseInfo prepareResponseInfo(int httpStatusCode) {
+        long urlRequestAdapter;
+        synchronized (mUrlRequestAdapterLock) {
+            urlRequestAdapter = mUrlRequestAdapter;

[mmenke, 2014/10/29 16:20:47]

This could be 0.  Could just put this entire function in a lock, and check if
it's 0 first thing.  If so, return null...and don't bother to run a task on the
UI thread.  Alternatively, could pass in a pointer from C++, and use that.  Nice
thing there is there's less redundant special case logic for the cancellation
case.  We do more work in that case, but doesn't matter - we only do it once per
request, at most, and it's not a huge amount of work.

[mef, 2014/10/29 19:05:00]

Done. C++ delegate doesn't get pointer to the adapter in the callbacks.

+        }
+        NativeResponseInfo responseInfo = new NativeResponseInfo(
+                mUrlChain.toArray(new String[mUrlChain.size()]),
+                httpStatusCode,
+                nativeGetWasCached(urlRequestAdapter),
+                nativeGetNegotiatedProtocol(urlRequestAdapter));
+        nativePopulateResponseHeaders(urlRequestAdapter,
+                                      responseInfo.mAllHeaders);
+        return responseInfo;
+    }
+
+    private void checkNotStarted() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter != 0) {

[mmenke, 2014/10/29 16:20:48]

We have no protection against being restarted after we delete the
mUrlRequestAdapter.  Suggest we add it, just as a precaution.

[mef, 2014/10/29 19:05:00]

Done.

+                throw new IllegalStateException("Request is already started.");
+            }
+        }
+    }
+
+    private void destroyRequestAdapter() {
+        synchronized (mUrlRequestAdapterLock) {
+            if (mUrlRequestAdapter != 0) {

[mmenke, 2014/10/29 16:20:47]

optional:  Suggest early return instead.  That's more in keeping with Chrome
style, and the other places you get this lock as well.

[mef, 2014/10/29 19:05:00]

Done.

+                nativeDestroyRequestAdapter(mUrlRequestAdapter);
+                mRequestContext.onRequestDestroyed(this);
+                mUrlRequestAdapter = 0;
+            }
+        }
+    }
+
+    private void appendHeaderToMap(HeadersMap headersMap,
+            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 cancelled
+     * and exception could be retrieved from request using getException().
+     */
+    private void onCalledByNativeException(Exception e) {
+        UrlRequestException requestError = new UrlRequestException(
+                "CalledByNative method has thrown an exception", e);
+        Log.e(CronetUrlRequestContext.LOG_TAG,
+                "Exception in CalledByNative method", e);
+        try {
+            cancel();
+            mListener.onError(this, mResponseInfo, requestError);

[mmenke, 2014/10/29 16:20:47]

We may not be on the executor at this point.  Should either make sure
onCalledByNativeException is run on the executor, or post a task here.

Also:  We should have tests that make sure, when there's a single-threaded
executor, all callbacks are executor on that executor.

Since our current tests use just such an executor, I suggest just updating the
current tests to all enforce that.

[mmenke, 2014/10/29 16:20:47]

While we're on the topic...  We really don't have any thread safety tests.  No
tests do anything on another thread.  It's very difficult to adequately test
thread safely, but we should have at least some tests that attempt to do so.

For example:  For the cancel during callback tests, we could have a variant
where we cancel on another thread, while blocking the callback until the cancel
task has been run.

[mmenke, 2014/10/29 16:24:35]

On second thought, that really doesn't test much...Need to think if we can come
up with something reasonable.

[mef, 2014/10/29 19:05:00]

The only caller not on executor is onAppendResponseHeader, I've added post
there.

+        } 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 cancelled 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) {

[mmenke, 2014/10/29 16:20:47]

Do we get anything out of the final here?  We could declare a bunch of other
values to be final, too (Like httpStatusCode).

[mef, 2014/10/29 19:05:00]

Compiler complains about newLocation as it is accessed from the posted task.

+        mUrlChain.add(newLocation);
+        final NativeResponseInfo responseInfo =
+                prepareResponseInfo(httpStatusCode);
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                try {
+                    mListener.onRedirect(CronetUrlRequest.this, responseInfo,
+                            new URL(newLocation));
+                    synchronized (mUrlRequestAdapterLock) {
+                        nativeFollowDeferredRedirect(mUrlRequestAdapter);

[mmenke, 2014/10/29 16:20:48]

Have to check if mUrlRequestAdapterLock is NULL - onRedirect could have
cancelled the request, or it could have been cancelled on another thread.

[mef, 2014/10/29 19:05:01]

Done.

+                    }
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        postAppTask(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) {
+                        nativeReceiveData(mUrlRequestAdapter);

[mmenke, 2014/10/29 16:20:47]

Have to check if mUrlRequestAdapterLock is NULL.

[mef, 2014/10/29 19:05:00]

Done.

+                    }
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        postAppTask(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;
+        postAppTask(mOnDataReceivedTask);
+    }
+
+    /**
+     * Called when request is complete, no callbacks will be called afterwards.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onComplete() {
+        long totalReceivedBytes;
+        synchronized (mUrlRequestAdapterLock) {
+            totalReceivedBytes =
+                    nativeGetTotalReceivedBytes(mUrlRequestAdapter);

[mmenke, 2014/10/29 16:20:48]

Have to check if mUrlRequestAdapter is NULL here.  Alternatively, could just ass
it in to onComplete from native code.

[mef, 2014/10/29 19:05:01]

Done.

+        }
+
+        final NativeExtendedResponseInfo extendedResponseInfo =
+                new NativeExtendedResponseInfo(mResponseInfo,
+                        totalReceivedBytes);
+        Runnable task = new Runnable() {
+            public void run() {
+                if (isCanceled()) {
+                    return;
+                }
+                try {
+                    mListener.onComplete(CronetUrlRequest.this,
+                                         extendedResponseInfo);
+                } catch (Exception e) {
+                    Log.e(CronetUrlRequestContext.LOG_TAG,
+                            "Exception in onComplete method", e);
+                }
+                destroyRequestAdapter();
+            }
+        };
+        postAppTask(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;
+                }
+                try {
+                    UrlRequestException requestError = new UrlRequestException(
+                            "Exception in CronetUrlRequest: " + errorString,
+                            nativeError);
+                    mListener.onError(CronetUrlRequest.this,
+                                      mResponseInfo,
+                                      requestError);
+                    destroyRequestAdapter();
+                } catch (Exception e) {
+                    onCalledByNativeException(e);
+                }
+            }
+        };
+        postAppTask(task);
+    }
+
+    /**
+     * Appends header |name| with value |value| to |headersMap|.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onAppendResponseHeader(HeadersMap headersMap,
+            String name, String value) {
+        try {
+            appendHeaderToMap(headersMap, name, value);
+        } catch (Exception e) {
+            onCalledByNativeException(e);
+        }
+    }
+
+    // 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 void 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>> {
     }
 }