components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequest.java - Issue 2844803002: [Cronet] Make metrics reporting happen after terminal callbacks.

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

Issue 2844803002: [Cronet] Make metrics reporting happen after terminal callbacks. (Closed)

Patch Set: self Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« no previous file with comments | « components/cronet/android/cronet_url_request_adapter.cc ('k') | components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

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..723b0d84cc2aed2f6849f3c8f3dddeb21f1f7609 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

+ // URLRequest.Callback executor's and RequestFinishedListener's thread after the last update.

mgersh 2017/05/02 18:21:12 nits: UrlRequest, RequestFinishedInfo.Listener. T

xunjieli 2017/05/02 23:30:23 Done.

+ @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,9 @@ 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.

+ // start() is synchronized so we do not acquire mUrlRequestAdapterLock here.

+ destroyRequestAdapterLocked(RequestFinishedInfo.FAILED);

throw e;

}

mStarted = true;

@@ -298,7 +301,7 @@ public final class CronetUrlRequest extends UrlRequestBase {

if (isDoneLocked() || !mStarted) {

return;

}

- destroyRequestAdapter(true);

+ destroyRequestAdapterLocked(RequestFinishedInfo.CANCELED);

}

@@ -335,7 +338,9 @@ public final class CronetUrlRequest extends UrlRequestBase {

@VisibleForTesting

public void setOnDestroyedCallbackForTesting(Runnable onDestroyedCallbackForTesting) {

- mOnDestroyedCallbackForTesting = onDestroyedCallbackForTesting;

+ synchronized (mUrlRequestAdapterLock) {

+ mOnDestroyedCallbackForTesting = onDestroyedCallbackForTesting;

+ }

}

@VisibleForTesting

@@ -410,18 +415,22 @@ 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();

- }

+ /**

+ * Helper method to set final status of CronetUrlRequest and clean up the

+ * native request adapter.

+ */

+ @GuardedBy("mUrlRequestAdapterLock")

+ private void destroyRequestAdapterLocked(

+ @RequestFinishedInfoImpl.FinishedReason int finishedReason) {

+ assert mException == null || finishedReason == RequestFinishedInfo.FAILED;

+ mFinishedReason = finishedReason;

+ if (mUrlRequestAdapter == 0) {

+ return;

}

+ mRequestContext.onRequestDestroyed();

+ // Posts a task to destroy the native adapter.

+ nativeDestroy(mUrlRequestAdapter, finishedReason == RequestFinishedInfo.CANCELED);

+ mUrlRequestAdapter = 0;

}

/**

@@ -447,31 +456,19 @@ 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);

+ assert mException == null;

+ mException = exception;

+ destroyRequestAdapterLocked(RequestFinishedInfo.FAILED);

}

+ // The onFailed callback will be invoked from onNativeAdapterDestroyed() to ensure metrics

+ // collection

mgersh 2017/05/02 18:21:13 nit: missing period at the end

xunjieli 2017/05/02 23:30:23 Done.

}

////////////////////////////////////////////////

@@ -600,7 +597,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 +607,13 @@ public final class CronetUrlRequest extends UrlRequestBase {

}

// Destroy adapter first, so request context could be shut

// down from the listener.

- destroyRequestAdapter(false);

+ destroyRequestAdapterLocked(RequestFinishedInfo.SUCCEEDED);

}

try {

mCallback.onSucceeded(CronetUrlRequest.this, mResponseInfo);

+ maybeReportMetrics();

} catch (Exception e) {

- Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onComplete method", e);

+ Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onSucceeded method", e);

}

};

@@ -637,7 +634,6 @@ 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);

}

@@ -657,12 +653,12 @@ 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);

+ maybeReportMetrics();

} catch (Exception e) {

Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in onCanceled method", e);

}

@@ -689,7 +685,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 +704,42 @@ public final class CronetUrlRequest extends UrlRequestBase {

pushEndMs, responseStartMs, requestEndMs, socketReused, sentByteCount,

receivedByteCount);

}

- mRequestContext.reportFinished(getRequestFinishedInfo());

+ // Metrics are reported to RequestFinishedListener when the final UrlRequest.Callback has

+ // been invoked.

}

- private RequestFinishedInfo getRequestFinishedInfo() {

- return new RequestFinishedInfoImpl(mInitialUrl, mRequestAnnotations, mMetrics,

- mFinishedReason, mResponseInfo, mException);

+ /**

+ * Called when the native adapter is destroyed.

+ */

+ @SuppressWarnings("unused")

+ @CalledByNative

+ private void onNativeAdapterDestroyed() {

+ synchronized (mUrlRequestAdapterLock) {

+ if (mOnDestroyedCallbackForTesting != null) {

+ mOnDestroyedCallbackForTesting.run();

+ }

+ // mException is set when an error is encountered (in native code via onError or in

+ // Java code). If mException is not null, notify the mCallback and report metrics.

+ if (mException == null) {

+ return;

+ }

+ Runnable task = new Runnable() {

+ @Override

+ public void run() {

+ try {

+ mCallback.onFailed(CronetUrlRequest.this, mResponseInfo, mException);

+ maybeReportMetrics();

+ } 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. */

@@ -752,6 +779,15 @@ public final class CronetUrlRequest extends UrlRequestBase {

}

+ // Maybe report metrics. This method should only be called on Callback's executor thread and

+ // after Callback's onSucceeded, onFailed and onCanceled.

+ private void maybeReportMetrics() {

+ if (mMetrics != null) {

+ mRequestContext.reportFinished(new RequestFinishedInfoImpl(mInitialUrl,

+ mRequestAnnotations, mMetrics, mFinishedReason, mResponseInfo, mException));

+ }

// Native methods are implemented in cronet_url_request_adapter.cc.

private native long nativeCreateRequestAdapter(long urlRequestContextAdapter, String url,