Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit eb461e20 authored by Tobias Thierer's avatar Tobias Thierer
Browse files

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
parent 97b74aa7

core/java/android/net/Network.java

+38 −35
Original line number Diff line number Diff line
@@ -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.

    // maybeInitUrlConnectionFactory() must be called prior to reading this field.

    private volatile HttpURLConnectionFactory mUrlConnectionFactory;

    // mUrlConnectionFactory is initialized lazily when it is first needed.

    @GuardedBy("mLock")

    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

    // 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.

    private void maybeInitUrlConnectionFactory() {

        synchronized (mLock) {

            if (mUrlConnectionFactory == null) {

    private static HttpURLConnectionFactory createUrlConnectionFactory(Dns dnsLookup) {

        // 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;

            }

        }

        return 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);

    }



    /**