[Cronet] Make metrics reporting happen after terminal callbacks.

components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
index eb2abaf83195efa16810c38ae43f3583407dbae8..ef6dca70ef1a0510efea34e80640912c645f6c0e 100644
--- a/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java
@@ -51,13 +51,11 @@ public final class CronetUrlRequest extends UrlRequestBase {
     private long mUrlRequestAdapter;
 
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mStarted = false;
+    private boolean mStarted;
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mWaitingOnRedirect = false;
+    private boolean mWaitingOnRedirect;
     @GuardedBy("mUrlRequestAdapterLock")
-    private boolean mWaitingOnRead = false;
-    @GuardedBy("mUrlRequestAdapterLock")
-    private RequestFinishedInfo.Metrics mMetrics;
+    private boolean mWaitingOnRead;
 
     /*
      * Synchronize access to mUrlRequestAdapter, mStarted, mWaitingOnRedirect,
@@ -81,9 +79,6 @@ public final class CronetUrlRequest extends UrlRequestBase {
     private String mInitialMethod;
     private final HeadersList mRequestHeaders = new HeadersList();
     private final Collection<Object> mRequestAnnotations;
-    @RequestFinishedInfoImpl.FinishedReason
-    private int mFinishedReason;
-    private CronetException mException;
     private final boolean mDisableCache;
     private final boolean mDisableConnectionMigration;
 
@@ -91,12 +86,20 @@ public final class CronetUrlRequest extends UrlRequestBase {
 
     private UrlResponseInfoImpl mResponseInfo;
 
+    // These three should only be updated once with mUrlRequestAdapterLock held.
+    // They are read on executor's thread after the last update.

[mef, 2017/04/28 21:16:54]

Should they be @GuardedBy("mUrlRequestAdapterLock")?

[mgersh, 2017/05/01 15:29:22]

this could clarify which executor

[xunjieli, 2017/05/01 19:17:16]

Done.

[xunjieli, 2017/05/01 19:17:16]

These three are not guarded by the lock because they are read on the executor's
thread without holding the lock. 

+    @RequestFinishedInfoImpl.FinishedReason
+    private int mFinishedReason;
+    private CronetException mException;
+    private CronetMetrics mMetrics;
+
     /*
      * Listener callback is repeatedly invoked when each read is completed, so it
      * is cached as a member variable.
      */
     private OnReadCompletedRunnable mOnReadCompletedTask;
 
+    @GuardedBy("mUrlRequestAdapterLock")
     private Runnable mOnDestroyedCallbackForTesting;
 
     private static final class HeadersList extends ArrayList<Map.Entry<String, String>> {}
@@ -234,9 +237,11 @@ public final class CronetUrlRequest extends UrlRequestBase {
                     return;
                 }
             } catch (RuntimeException e) {
-                // If there's an exception, cleanup and then throw the
-                // exception to the caller.
-                destroyRequestAdapter(false);
+                // If there's an exception, cleanup and then throw the exception to the caller. Use
+                // null to avoid rethrowing in onDestroy().
+                synchronized (mUrlRequestAdapterLock) {

[mef, 2017/04/28 21:16:54]

This whole method is synchronized() at line 191, so I don't think this is needed
here.

[xunjieli, 2017/05/01 19:17:16]

Done.

+                    setIsDoneLocked(null, RequestFinishedInfo.FAILED);
+                }
                 throw e;
             }
             mStarted = true;
@@ -298,7 +303,7 @@ public final class CronetUrlRequest extends UrlRequestBase {
             if (isDoneLocked() || !mStarted) {
                 return;
             }
-            destroyRequestAdapter(true);
+            setIsDoneLocked(null, RequestFinishedInfo.CANCELED);
         }
     }
 
@@ -309,6 +314,26 @@ public final class CronetUrlRequest extends UrlRequestBase {
         }
     }
 
+    /**
+     * Helper method to set final status of CronetUrlRequest and clean up the
+     * native request adapter.
+     */
+    @GuardedBy("mUrlRequestAdapterLock")
+    private void setIsDoneLocked(

[mef, 2017/04/28 21:16:55]

I really think it should still be called destroyRequestAdapterLocked as that's
what it does. 

[xunjieli, 2017/05/01 19:17:16]

Done. Partly yes, but now it also updates |mException| and |mFinishedReason|. I
think it will be a nice counterpart to isDone(). I guess we could discuss that
in a separate CL. I have changed it back.

+            CronetException exception, @RequestFinishedInfoImpl.FinishedReason int finishedReason) {
+        assert mException == null;
+        assert exception == null || finishedReason == RequestFinishedInfo.FAILED;
+        mException = exception;
+        mFinishedReason = finishedReason;
+        if (mUrlRequestAdapter == 0) {
+            return;
+        }
+        mRequestContext.onRequestDestroyed();
+        // Posts a task to destroy the native adapter.
+        nativeDestroy(mUrlRequestAdapter, finishedReason == RequestFinishedInfo.CANCELED);
+        mUrlRequestAdapter = 0;
+    }
+
     @GuardedBy("mUrlRequestAdapterLock")
     private boolean isDoneLocked() {
         return mStarted && mUrlRequestAdapter == 0;
@@ -335,7 +360,9 @@ public final class CronetUrlRequest extends UrlRequestBase {
 
     @VisibleForTesting
     public void setOnDestroyedCallbackForTesting(Runnable onDestroyedCallbackForTesting) {
-        mOnDestroyedCallbackForTesting = onDestroyedCallbackForTesting;
+        synchronized (mUrlRequestAdapterLock) {
+            mOnDestroyedCallbackForTesting = onDestroyedCallbackForTesting;
+        }
     }
 
     @VisibleForTesting
@@ -410,20 +437,6 @@ public final class CronetUrlRequest extends UrlRequestBase {
         }
     }
 
-    private void destroyRequestAdapter(boolean sendOnCanceled) {
-        synchronized (mUrlRequestAdapterLock) {
-            if (mUrlRequestAdapter == 0) {
-                return;
-            }
-            nativeDestroy(mUrlRequestAdapter, sendOnCanceled);
-            mRequestContext.onRequestDestroyed();
-            mUrlRequestAdapter = 0;
-            if (mOnDestroyedCallbackForTesting != null) {
-                mOnDestroyedCallbackForTesting.run();
-            }
-        }
-    }
-
     /**
      * If callback method throws an exception, request gets canceled
      * and exception is reported via onFailed listener callback.
@@ -447,31 +460,17 @@ public final class CronetUrlRequest extends UrlRequestBase {
     }
 
     /**
-     * Fails the request with an exception. Can be called on any thread.
+     * Fails the request with an exception on any thread.
      */
     private void failWithException(final CronetException exception) {
-        mException = exception;
         synchronized (mUrlRequestAdapterLock) {
             if (isDoneLocked()) {
                 return;
             }
-            destroyRequestAdapter(false);
-        }
-        Runnable task = new Runnable() {
-            @Override
-            public void run() {
-                try {
-                    mCallback.onFailed(CronetUrlRequest.this, mResponseInfo, exception);
-                } catch (Exception e) {
-                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onFailed method", e);
-                }
-            }
-        };
-        try {
-            mExecutor.execute(task);
-        } catch (RejectedExecutionException e) {
-            Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor", e);
+            setIsDoneLocked(exception, RequestFinishedInfo.FAILED);

[mef, 2017/04/28 21:16:54]

Can we set mException = exception; here and just pass RequestFinishedInfo.Reason
to destroyRequestAdapterLocked()?
This way other callers wouldn't need to pass null.

[xunjieli, 2017/05/01 19:17:16]

Done.

         }
+        // Wait until onDestroy() is called to propagate the error to caller to

[mef, 2017/04/28 21:16:54]

I would rephrase this like 'The onFailed callback will be invoked from
onNativeAdapterDestroyed() to ensure metrics collection.

[xunjieli, 2017/05/01 19:17:16]

Done.

+        // gather metrics.
     }
 
     ////////////////////////////////////////////////
@@ -600,7 +599,6 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @SuppressWarnings("unused")
     @CalledByNative
     private void onSucceeded(long receivedByteCount) {
-        mFinishedReason = RequestFinishedInfo.SUCCEEDED;
         mResponseInfo.setReceivedByteCount(mReceivedByteCountFromRedirects + receivedByteCount);
         Runnable task = new Runnable() {
             @Override
@@ -611,12 +609,17 @@ public final class CronetUrlRequest extends UrlRequestBase {
                     }
                     // Destroy adapter first, so request context could be shut
                     // down from the listener.
-                    destroyRequestAdapter(false);
+                    setIsDoneLocked(null, RequestFinishedInfo.SUCCEEDED);
                 }
                 try {
                     mCallback.onSucceeded(CronetUrlRequest.this, mResponseInfo);
+                    if (mMetrics != null) {
+                        mRequestContext.reportFinished(
+                                new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations,
+                                        mMetrics, mFinishedReason, mResponseInfo, mException));
+                    }
                 } catch (Exception e) {
-                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onComplete method", e);
+                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onSucceeded method", e);
                 }
             }
         };
@@ -637,18 +640,19 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @CalledByNative
     private void onError(int errorCode, int nativeError, int nativeQuicError, String errorString,
             long receivedByteCount) {
-        mFinishedReason = RequestFinishedInfo.FAILED;
         if (mResponseInfo != null) {
             mResponseInfo.setReceivedByteCount(mReceivedByteCountFromRedirects + receivedByteCount);
         }
+        CronetException exception;

[mef, 2017/04/28 21:16:54]

Why do we need local |exception| here?

[xunjieli, 2017/05/01 19:17:16]

Done. Because we can acquire mUrlRequestAdapterLock once for all in
failWithException() ? I got rid of it.

         if (errorCode == NetworkException.ERROR_QUIC_PROTOCOL_FAILED) {
-            failWithException(new QuicExceptionImpl(
-                    "Exception in CronetUrlRequest: " + errorString, nativeError, nativeQuicError));
+            exception = new QuicExceptionImpl(
+                    "Exception in CronetUrlRequest: " + errorString, nativeError, nativeQuicError);
         } else {
             int javaError = mapUrlRequestErrorToApiErrorCode(errorCode);
-            failWithException(new NetworkExceptionImpl(
-                    "Exception in CronetUrlRequest: " + errorString, javaError, nativeError));
+            exception = new NetworkExceptionImpl(
+                    "Exception in CronetUrlRequest: " + errorString, javaError, nativeError);
         }
+        failWithException(exception);
     }
 
     /**
@@ -657,12 +661,16 @@ public final class CronetUrlRequest extends UrlRequestBase {
     @SuppressWarnings("unused")
     @CalledByNative
     private void onCanceled() {
-        mFinishedReason = RequestFinishedInfo.CANCELED;
         Runnable task = new Runnable() {
             @Override
             public void run() {
                 try {
                     mCallback.onCanceled(CronetUrlRequest.this, mResponseInfo);
+                    if (mMetrics != null) {

[mef, 2017/04/28 21:16:54]

Does it make sense to extract this into maybeReportFinished or somesuch from
here and in onSucceeded and onDestroyed? 

[xunjieli, 2017/05/01 19:17:16]

Done.

+                        mRequestContext.reportFinished(
+                                new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations,
+                                        mMetrics, mFinishedReason, mResponseInfo, mException));
+                    }
                 } catch (Exception e) {
                     Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onCanceled method", e);
                 }
@@ -689,7 +697,8 @@ public final class CronetUrlRequest extends UrlRequestBase {
     }
 
     /**
-     * Called by the native code to report metrics.
+     * Called by the native code on the network thread to report metrics. Happens before
+     * onSucceeded, onError and onCanceled.
      */
     @SuppressWarnings("unused")
     @CalledByNative
@@ -707,12 +716,42 @@ public final class CronetUrlRequest extends UrlRequestBase {
                     pushEndMs, responseStartMs, requestEndMs, socketReused, sentByteCount,
                     receivedByteCount);
         }

[mef, 2017/04/28 21:16:54]

Maybe add comment that metrics will be reported after final callback?

[xunjieli, 2017/05/01 19:17:16]

Done.

-        mRequestContext.reportFinished(getRequestFinishedInfo());
     }
 
-    private RequestFinishedInfo getRequestFinishedInfo() {
-        return new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations, mMetrics,
-                mFinishedReason, mResponseInfo, mException);
+    /**
+     * Called when the native adapter is destroyed.
+     */
+    @SuppressWarnings("unused")
+    @CalledByNative
+    private void onDestroyed() {

[mef, 2017/04/28 21:16:55]

Maybe (not strongly) call it onNativeAdapterDestroyed?

[xunjieli, 2017/05/01 19:17:16]

Done.

+        synchronized (mUrlRequestAdapterLock) {
+            if (mOnDestroyedCallbackForTesting != null) {
+                mOnDestroyedCallbackForTesting.run();
+            }
+            if (mException == null) {

[mef, 2017/04/28 21:16:55]

Need a comment as to why  we check mException and possibly post onFailed from
here.

[xunjieli, 2017/05/01 19:17:16]

Done.

+                return;
+            }
+        }
+        Runnable task = new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    mCallback.onFailed(CronetUrlRequest.this, mResponseInfo, mException);
+                    if (mMetrics != null) {
+                        mRequestContext.reportFinished(
+                                new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations,
+                                        mMetrics, mFinishedReason, mResponseInfo, mException));
+                    }
+                } catch (Exception e) {
+                    Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onFailed method", e);
+                }
+            }
+        };
+        try {
+            mExecutor.execute(task);
+        } catch (RejectedExecutionException e) {
+            Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor", e);
+        }
     }
 
     /** Enforces prohibition of direct execution. */