New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 656765 link

Starred by 1 user
Status: WontFix
Owner:
tbansal@chromium.org
Closed: Oct 2016
Cc:
ryansturm@chromium.org
█ clm@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Determine the optimal locking order for NQE APIs in Cronet

Project Member Reported by tbansal@chromium.org, Oct 17 2016 
CronetUrlRequestContext.java currently uses nested locks: First it acquires mNetworkQualityLock followed by mLock, and later it releases both locks at the same time.

It is not clear why the NQE APIs need nested locks. A faster approach is to first acquire mLock, check if a valid adapter is present, release mLock. Next, acquire and release the mNetworkQualityLock.

This nested lock pattern was first added in https://codereview.chromium.org/2283243002/diff/120001/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java. So, CC'ing clm@ who might have more insights.

Comment 1 by clm@google.com, Oct 18 2016 

I was making changes in CronetUrlRequestContext, and I added @GuardedBy so that the compiler would check my locking; this revealed a bug, which was that:

checkHaveAdapter() calls haveRequestContextAdapter which inspects mUrlRequestContextAdapter, which should only be accessed when holding mLock.

The java memory model permits unsynchronized access of long fields to observe a sheared value (as in, a value that was never actually set to that field), and so it's important to synchronize.

Comment 2 by tbansal@chromium.org, Oct 21 2016

Status: WontFix (was: Assigned)
mLock was removed from NQE APIs in https://codereview.chromium.org/2439833002/. Marking as WontFix.

Comment 3 by tbansal@chromium.org, Oct 21 2016

Labels: NQE

Comment 4 by tbansal@chromium.org, Feb 13 2017

Components: Internals>Network>NetworkQuality

Comment 5 by tbansal@chromium.org, Feb 14 2017

Labels: -nqe
►

Sign in to add a comment