Refactor Network's use of mUrlConnectionFactory.

1.) Replace maybeInitUrlConnectionFactory() with a static method
    createUrlConnectionFactory(Dns) and move the logic to acquire
    the lock and initialize mUrlConnectionFactory out into
    openConnection. This makes it a bit clearer that the lock
    is acquired during openConnection().
2.) Use @GuardedBy("mLock") rather than a human readable comment
    on mUrlConnectionFactory.
3.) Make mUrlConnectionFactory non-volatile (since it's guarded by
    mLock), as recommended by Narayan on the review thread for
    http://r.android.com/370652
    Alternatively, the field could have remained volatile
    and we could have used double-checked locking to avoid
    acquiring the lock in the common case.
    The lock is only acquired during getSocketFactory() and
    openConnection(), so it shouldn't usually be contended.

This CL is a pure refactoring that shouldn't have any observable
behavior change.

Bug: 38311512
Test: Treehugger
Exempt-From-Owner-Approval: refactoring only, owner didn't respond in time
Change-Id: I1cf6075dc7cd994657b11d6a82de3ec63235fb1e

core/java/android/net/Network.java

@@ -27,6 +27,7 @@ import android.system.Os;
 import android.system.OsConstants;
 import android.util.proto.ProtoOutputStream;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.okhttp.internalandroidapi.Dns;
 import com.android.okhttp.internalandroidapi.HttpURLConnectionFactory;
 
@@ -70,9 +71,9 @@ public class Network implements Parcelable {
     // Objects used to perform per-network operations such as getSocketFactory
     // and openConnection, and a lock to protect access to them.
     private volatile NetworkBoundSocketFactory mNetworkBoundSocketFactory = null;
-    // mLock should be used to control write access to mUrlConnectionFactory.
+    // mUrlConnectionFactory is initialized lazily when it is first needed.
-    // maybeInitUrlConnectionFactory() must be called prior to reading this field.
+    @GuardedBy("mLock")
-    private volatile HttpURLConnectionFactory mUrlConnectionFactory;
+    private HttpURLConnectionFactory mUrlConnectionFactory;
     private final Object mLock = new Object();
 
     // Default connection pool values. These are evaluated at startup, just
@@ -284,36 +285,16 @@ public class Network implements Parcelable {
         return mNetworkBoundSocketFactory;
     }
 
-    // TODO: This creates a connection pool and host resolver for
+    private static HttpURLConnectionFactory createUrlConnectionFactory(Dns dnsLookup) {
-    // every Network object, instead of one for every NetId. This is
+        // Set configuration on the HttpURLConnectionFactory that will be good for all
-    // suboptimal, because an app could potentially have more than one
+        // connections created by this Network. Configuration that might vary is left
-    // Network object for the same NetId, causing increased memory footprint
+        // until openConnection() and passed as arguments.
-    // and performance penalties due to lack of connection reuse (connection
+        HttpURLConnectionFactory urlConnectionFactory = new HttpURLConnectionFactory();
-    // setup time, congestion window growth time, etc.).
+        urlConnectionFactory.setDns(dnsLookup); // Let traffic go via dnsLookup
-    //
+        // A private connection pool just for this Network.
-    // Instead, investigate only having one connection pool and host resolver
+        urlConnectionFactory.setNewConnectionPool(httpMaxConnections,
-    // for every NetId, perhaps by using a static HashMap of NetIds to
+                httpKeepAliveDurationMs, TimeUnit.MILLISECONDS);
-    // connection pools and host resolvers. The tricky part is deciding when
+        return urlConnectionFactory;
-    // to remove a map entry; a WeakHashMap shouldn't be used because whether
-    // a Network is referenced doesn't correlate with whether a new Network
-    // will be instantiated in the near future with the same NetID. A good
-    // solution would involve purging empty (or when all connections are timed
-    // out) ConnectionPools.
-    private void maybeInitUrlConnectionFactory() {
-        synchronized (mLock) {
-            if (mUrlConnectionFactory == null) {
-                // Set configuration on the HttpURLConnectionFactory that will be good for all
-                // connections created by this Network. Configuration that might vary is left
-                // until openConnection() and passed as arguments.
-                Dns dnsLookup = hostname -> Arrays.asList(Network.this.getAllByName(hostname));
-                HttpURLConnectionFactory urlConnectionFactory = new HttpURLConnectionFactory();
-                urlConnectionFactory.setDns(dnsLookup); // Let traffic go via dnsLookup
-                // A private connection pool just for this Network.
-                urlConnectionFactory.setNewConnectionPool(httpMaxConnections,
-                        httpKeepAliveDurationMs, TimeUnit.MILLISECONDS);
-                mUrlConnectionFactory = urlConnectionFactory;
-            }
-        }
     }
 
     /**
@@ -354,9 +335,31 @@ public class Network implements Parcelable {
      */
     public URLConnection openConnection(URL url, java.net.Proxy proxy) throws IOException {
         if (proxy == null) throw new IllegalArgumentException("proxy is null");
-        maybeInitUrlConnectionFactory();
+        // TODO: This creates a connection pool and host resolver for
+        // every Network object, instead of one for every NetId. This is
+        // suboptimal, because an app could potentially have more than one
+        // Network object for the same NetId, causing increased memory footprint
+        // and performance penalties due to lack of connection reuse (connection
+        // setup time, congestion window growth time, etc.).
+        //
+        // Instead, investigate only having one connection pool and host resolver
+        // for every NetId, perhaps by using a static HashMap of NetIds to
+        // connection pools and host resolvers. The tricky part is deciding when
+        // to remove a map entry; a WeakHashMap shouldn't be used because whether
+        // a Network is referenced doesn't correlate with whether a new Network
+        // will be instantiated in the near future with the same NetID. A good
+        // solution would involve purging empty (or when all connections are timed
+        // out) ConnectionPools.
+        final HttpURLConnectionFactory urlConnectionFactory;
+        synchronized (mLock) {
+            if (mUrlConnectionFactory == null) {
+                Dns dnsLookup = hostname -> Arrays.asList(getAllByName(hostname));
+                mUrlConnectionFactory = createUrlConnectionFactory(dnsLookup);
+            }
+            urlConnectionFactory = mUrlConnectionFactory;
+        }
         SocketFactory socketFactory = getSocketFactory();
-        return mUrlConnectionFactory.openConnection(url, socketFactory, proxy);
+        return urlConnectionFactory.openConnection(url, socketFactory, proxy);
     }
 
     /**