Cronet: a framework for providing alternative Cronet implementations

components/cronet/android/api/src/org/chromium/net/CronetEngine.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
 import android.content.Context;
 import android.net.http.HttpResponseCache;
+import android.support.annotation.VisibleForTesting;
 
 import java.io.IOException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
+import java.util.Iterator;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.Executor;
 
 import javax.net.ssl.HttpsURLConnection;
 /**
  * An engine to process {@link UrlRequest}s, which uses the best HTTP stack
  * available on the current platform. An instance of this class can be created
  * using {@link Builder}.
  */
 public abstract class CronetEngine {
@@ skipping 31 matching lines @@
          * {@link CronetEngine}. The default configuration enables HTTP/2 and
          * disables QUIC, SDCH and the HTTP cache.
          *
          * @param context Android {@link Context}, which is used by
          *                {@link Builder} to retrieve the application
          *                context. A reference to only the application
          *                context will be kept, so as to avoid extending
          *                the lifetime of {@code context} unnecessarily.
          */
         public Builder(Context context) {
-            mBuilderDelegate = ImplLoader.load(context);
+            this(createBuilderDelegate(context));
         }
 
         /**
+         * Constructs {@link Builder} with a given delegate that provides the actual implementation
+         * of the {@code Builder} methods. This constructor is used only by the internal
+         * implementation.
+         *
+         * @param builderDelegate delegate that provides the actual implementation.
+         *
+         * {@hide}
+         */
+        public Builder(ICronetEngineBuilder builderDelegate) {
+            mBuilderDelegate = builderDelegate;
+        }
+
+        /**
          * Constructs a User-Agent string including application name and version,
          * system build version, model and id, and Cronet version.
          *
          * @return User-Agent string.
          */
         public String getDefaultUserAgent() {
             return mBuilderDelegate.getDefaultUserAgent();
         }
 
         /**
@@ skipping 195 matching lines @@
             return this;
         }
 
         /**
          * Build a {@link CronetEngine} using this builder's configuration.
          * @return constructed {@link CronetEngine}.
          */
         public CronetEngine build() {
             return mBuilderDelegate.build();
         }
+
+        /**
+         * Creates an implementation of {@link ICronetEngineBuilder} by going through
+         * all available {@link CronetProvider CronetProviders} and selecting one
+         * with the highest priority.

[pauljensen, 2017/01/20 17:13:35]

Please explain "highest priority"

[kapishnikov, 2017/01/20 21:48:38]

Since an enabled provider should never return null. This method implementation
has been changed to consider only the first element of the list. Changed the
javadoc accordingly.

+         *
+         * @param context Android Context to use.
+         * @return the created {@code ICronetEngineBuilder}.
+         */
+        private static ICronetEngineBuilder createBuilderDelegate(Context context) {
+            ICronetEngineBuilder mBuilderDelegate;

[pauljensen, 2017/01/20 17:13:35]

local variables should not have "m" prefixes, remove this variable.

[kapishnikov, 2017/01/20 21:48:38]

Deleted.

+            List<CronetProvider> providerList =
+                    getEnabledCronetProviders(context, CronetProviders.getInstance());
+
+            // Try to load implementation using available providers. Assuming that

[mef, 2017/01/19 22:57:46]

nit: Assuming -> Assume

[kapishnikov, 2017/01/20 16:21:53]

Done.

+            // providers in the beginning of the list have higher priority.
+            ICronetEngineBuilder builderDelegate = null;
+            for (CronetProvider provider : providerList) {
+                Builder builder = provider.createBuilder();
+                if (builder != null) {

[pauljensen, 2017/01/20 17:13:35]

We should use either isEnabled() or createBuilder() returning non-null to signal
an enabled provider, not both.  I vote for isEnabled(), so we can get rid of
this null check and just write:
builderDelegate = provider.createBuilder().mBuilderDelegate;

[kapishnikov, 2017/01/20 21:48:38]

Agree. createBuilder() now throws an exception that should be propagated to the
embedder.

+                    builderDelegate = builder.mBuilderDelegate;
+                    break;
+                }
+            }
+
+            if (builderDelegate == null) {
+                throw new RuntimeException("Unable to load any Cronet implementation."
+                        + " The list of enabled providers: " + providerList);
+            }
+
+            mBuilderDelegate = builderDelegate;
+            return mBuilderDelegate;
+        }
+
+        /**
+         * Returns the list of available and enabled {@link CronetProvider}. The returned list
+         * is sorted based on the provider versions and types.
+         *
+         * @param context Android Context to use.
+         * @return the sorted list of enabled providers.
+         * @throws RuntimeException is the list of providers is empty or all of the providers
+         *                          are disabled.
+         *
+         * {@hide}

[pauljensen, 2017/01/20 17:13:35]

I don't think package private appears in javadocs, so no {@hide} needed.

[kapishnikov, 2017/01/20 21:48:38]

Removed.

+         */
+        @VisibleForTesting
+        static List<CronetProvider> getEnabledCronetProviders(

[mef, 2017/01/19 22:57:46]

Should this and below be moved into CronetProviders? 
Then all sorting / enabled / available logic is located in one place?

Per offline discussion I don't feel strongly and can be convinced otherwise.
Paul, WDYT?

[kapishnikov, 2017/01/20 16:21:53]

I have a strong opinion that CronetProviders should only be responsible for
providing the list of all Cronet providers available by the app. It is
responsibility of the client of the class to select the right one.

+                Context context, CronetProviders providers) {
+            // Check that there is at least one available provider.
+            List<CronetProvider> providerList = providers.getAvailableProviders(context);
+            if (providerList.size() == 0) {
+                throw new RuntimeException("Unable to find any Cronet provider."
+                        + " Have you included all necessary jars?");
+            }
+
+            // Exclude disabled providers from the list.
+            for (Iterator<CronetProvider> i = providerList.iterator(); i.hasNext();) {
+                CronetProvider provider = i.next();
+                if (!provider.isEnabled()) {
+                    i.remove();
+                }
+            }
+
+            // Check that there is at least one enabled provider.
+            if (providerList.size() == 0) {
+                throw new RuntimeException("All found Cronet providers are disabled."

[mef, 2017/01/19 22:57:46]

nit: found -> available

[kapishnikov, 2017/01/20 16:21:53]

Done.

+                        + " A provider should be enabled before it can be used.");
+            }
+
+            // Sort providers based on version and type.
+            Collections.sort(providerList, new Comparator<CronetProvider>() {
+                @Override
+                public int compare(CronetProvider p1, CronetProvider p2) {
+                    // The platform provider should always be at the end of the list.

[mef, 2017/01/19 22:57:46]

Can PlatformProvider be just added to the end of the list if it is available?

[kapishnikov, 2017/01/20 16:21:53]

We could extract PlatformProvider from the list, sort it and add it to the end
of the list. I don't think it is easier way to do the sorting; unless we change
CronetProviders interface to add some PlatformProvider specific logic.

+                    if (CronetProviders.PROVIDER_NAME_PLATFORM.equals(p1.getName())) {
+                        return 1;
+                    }
+                    if (CronetProviders.PROVIDER_NAME_PLATFORM.equals(p2.getName())) {
+                        return -1;
+                    }
+                    // A provider with higher version should go first.
+                    return -compareVersions(p1.getVersion(), p2.getVersion());

[mef, 2017/01/19 22:57:46]

Is it ok if compareVersions throws an exception?

[kapishnikov, 2017/01/20 16:21:53]

It is a good question. Currently it will cause the app stop with the exception.
I think that is what we should do since if a provider has an invalid version
something is wrong.

+                }
+            });
+            return providerList;
+        }
+
+        /**
+         * Compares two strings that contain versions. The string should only contain
+         * dot-separated segments that contain an arbitrary number of digits digits [0-9].
+         *
+         * @param s1 the first string.
+         * @param s2 the second string.
+         * @return -1 if s1<s2, +1 if s1>s2 and 0 if s1=s2. If two versions are equal, the
+         *         version with the higher number of segments is considered to be higher.
+         *
+         * @throws IllegalArgumentException if any of the strings contains an illegal
+         * version number.
+         *
+         * {@hide}

[pauljensen, 2017/01/20 17:13:35]

I don't think package private appears in javadocs, so no {@hide} needed.

[kapishnikov, 2017/01/20 21:48:38]

Removed.

+         */
+        @VisibleForTesting
+        static int compareVersions(String s1, String s2) {
+            if (s1 == null || s2 == null) {
+                throw new IllegalArgumentException("The input values cannot be null");
+            }
+            String[] s1segments = s1.split("\\.");
+            String[] s2segments = s2.split("\\.");
+            int index = 0;
+            while (s1segments.length > index && s2segments.length > index) {

[pauljensen, 2017/01/20 17:13:35]

how about:
for (int i = 0; i < s1segments.length && i < s2segments.length; i++) {

[kapishnikov, 2017/01/20 21:48:38]

Neat. Done.

+                try {
+                    int s1segment = Integer.parseInt(s1segments[index]);
+                    int s2segment = Integer.parseInt(s2segments[index]);
+                    if (s1segment != s2segment) {
+                        return Integer.signum(s1segment - s2segment);
+                    }
+                    index += 1;
+                } catch (NumberFormatException e) {
+                    throw new IllegalArgumentException("Unable to convert version segments into"
+                                    + " integers: " + s1segments[index] + " & " + s2segments[index],
+                            e);
+                }
+            }
+            return Integer.signum(s1segments.length - s2segments.length);
+        }
     }
 
     /**
      * @return a human-readable version string of the engine.
      */
     public abstract String getVersionString();
 
     /**
      * Shuts down the {@link CronetEngine} if there are no active requests,
      * otherwise throws an exception.
@@ skipping 104 matching lines @@
      * thread calling {@link Executor#execute} to prevent blocking networking
      * operations and causing exceptions during shutdown.
      *
      * @param url URL for the generated requests.
      * @param callback callback object that gets invoked on different events.
      * @param executor {@link Executor} on which all callbacks will be invoked.
      */
     public abstract UrlRequest.Builder newUrlRequestBuilder(
             String url, UrlRequest.Callback callback, Executor executor);
 }