HttpClient configuration #12423

tobrun · 2018-07-18T13:48:45Z

WIP:

This PR allows to exclude the okhttp dependency from our SDK and replace the http client integration with another. In the test app I'm now showcasing this with using ion instead of okhttp.

Notes:

I had to disable telemetry completely as I was hitting NoClassDefFoundError, stripping out okhttp also removes it from telemetry. We need an additional configuration handling this.
I had to introduce a collection to manage ongoing requests, with the old system this wasn't necessary but with this static approach we need additional bookkeeping.

Todo:

revisit naming
add javadoc
fix telemetry integration
remove ion sample integration from testapp
run large scale tests on AWS device farm to detect regressions

zugaldia · 2018-07-18T13:54:16Z

I had to disable telemetry completely as I was hitting NoClassDefFoundError, stripping out okhttp also removes it from telemetry. We need an additional configuration handling this.

cc: @electrostat for coordination.

LukasPaczos

Amazing work so far!

Idea - how about using generics on the calls/requests list, managing that in the abstract 'HttpRequest` and only requesting call creation and cancellation from the child?

LukasPaczos · 2018-07-18T15:20:55Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/OkHttpRequest.java

+          currentCall = calls.get(key);
+          if (call.equals(currentCall)) {
+            calls.delete(key);
+          }


I believe we can safely return here.

yes, also updated code block to:
Which should also be faster as it avoids using get(key)

synchronized (calls) { Call currentCall; for (int i = 0; i < calls.size(); i++) { currentCall = calls.valueAt(i); if (call.equals(currentCall)) { calls.delete(calls.keyAt(i)); return; } } }

LukasPaczos · 2018-07-18T15:23:49Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/OkHttpRequest.java

+      Request.Builder builder = new Request.Builder()
+        .url(resourceUrl)
+        .tag(resourceUrl.toLowerCase(MapboxConstants.MAPBOX_LOCALE))
+        .addHeader("User-Agent", getUserAgent());


Do we need lazy init here instead of initialization on the object creation?

this was a remains of the old implementation, updated to create this inline in the class field.

LukasPaczos · 2018-07-18T15:25:53Z

...TestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/SimpleMapActivity.java

+    mapView.getMapAsync(new OnMapReadyCallback() {
+      @Override
+      public void onMapReady(MapboxMap mapboxMap) {
+        mapboxMap.getUiSettings().setCompassFadeFacingNorth(false);


Changes in SimpleMapActivity.java seem unnecessary.

should be removed, that was a test for a different issue

LukasPaczos · 2018-07-18T15:38:36Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java

+
+  public abstract void cancelRequest(long nativePtr);
+
+  public static void execute(NativeHttpRequest httpRequest, long nativePtr, String resourceUrl,


Can this be package-private?

yes it can, great catch

LukasPaczos · 2018-07-18T15:46:51Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java

+  static final int PERMANENT_ERROR = 2;
+
+  public static synchronized void setHttpRequest(@Nullable HttpRequest requestImpl) {
+    httpRequest = requestImpl;


I think we need to invoke a cancelAll method here because we will lose the reference to the calls when the request impl is changed.

The idea of this PR is to be able exclude okhttp, a cancelAll invocation in this code path on the OkHttpRequest will result in the initiating OkHttpRequest and the classloader will hit a NoClassDefFoundError.

imo switching out httpclients on the fly isn't a supported use-case. You either use the default okhttp implementation or you provide an alternate implementation before requesting any resources.

I'm down with different suggestions to tackle this but don't think there is much we can do other than documenting this clearly in the javadoc.

Right, let's document it extensively then.

What about throwing an exception when resources were already requested and a user wants to switch the implementation?

LukasPaczos · 2018-07-18T15:48:08Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java

+    getInstance().executeRequest(httpRequest, nativePtr, resourceUrl, etag, modified);
+  }
+
+  public static void cancel(long nativePtr) {


Can this be package-private?

yes it can, great catch

electrostat · 2018-07-18T17:23:46Z

@tobrun @zugaldia Regarding Telemetry, we need a non-okhttp solution (similar to what you have here) to perform requests and allow that to be parent sdk settable? Am i correct in that assumption? If so, I can start working on a solution for this.

tobrun · 2018-07-19T07:31:04Z

@LukasPaczos

Idea - how about using generics on the calls/requests list, managing that in the abstract 'HttpRequest` and only requesting call creation and cancellation from the child?

I liked this idea but isn't possible due to the call list needing to be static (to be shared between instances of HttpRequest) and you can't reference the generic type in a static context. Please point out if I'm missing something.

LukasPaczos · 2018-07-19T10:28:29Z

Yes, you are right, this would require an additional, non-static container for all the requests. Not sure if this is worth pursuing as the current setup allows more flexibility at the cost of complexity. I feel like this is a good tradeoff in this case.

jaegs · 2018-07-19T17:52:39Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/OkHttpRequest.java

+        log(Log.ERROR, String.format("[HTTP] Unable to parse resourceUrl %s", resourceUrl));
+      }
+
+      final String host = httpUrl.host().toLowerCase(MapboxConstants.MAPBOX_LOCALE);


Logic like computing the host is not specific to OkHttp so I think it should be available in a delegate.

jaegs · 2018-07-19T17:55:08Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java

+
+  private static synchronized HttpRequest getInstance() {
+    if (httpRequest == null) {
+      httpRequest = new OkHttpRequest();


Part of the goal of having modules is that OkHttp does not need to be compiled into the APK. I think there should be a DefaultModules.java file that sets all the default modules. Then for someone who wants to have different modules, they just leave DefaultModules out of the compilation and make their own MyModules.

I'm aware of two techniques to remove code from an aar/binary:

gradle exclusion

proguard

Please elaborate how they just leave DefaultModules out of the compilation and make their own MyModules.

I was thinking an option in the build rules to leave DefaultModules.java out - so I think that would be gradle exclusion though I'm not actually familiar with gradle.

tobrun · 2018-07-20T16:55:05Z

Moving forward in #12449 where we opt to use a compile time configuration vs runtime.

tobrun added the Android Mapbox Maps SDK for Android label Jul 18, 2018

tobrun added this to the android-v6.4.0 milestone Jul 18, 2018

tobrun self-assigned this Jul 18, 2018

zugaldia requested a review from LukasPaczos July 18, 2018 13:53

tobrun force-pushed the tvn-configurable-httpclient branch from 6f06a7b to 599f37b Compare July 18, 2018 14:04

LukasPaczos reviewed Jul 18, 2018

View reviewed changes

[android] - make used http client configurable

a7de858

tobrun force-pushed the tvn-configurable-httpclient branch from 599f37b to a7de858 Compare July 19, 2018 10:01

jaegs reviewed Jul 19, 2018

View reviewed changes

tobrun mentioned this pull request Jul 20, 2018

HttpClient configuration with modules #12449

Closed

tobrun closed this Jul 20, 2018

tobrun deleted the tvn-configurable-httpclient branch July 20, 2018 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpClient configuration #12423

HttpClient configuration #12423

tobrun commented Jul 18, 2018

zugaldia commented Jul 18, 2018

LukasPaczos left a comment

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 19, 2018

tobrun Jul 19, 2018

LukasPaczos Jul 18, 2018

tobrun Jul 19, 2018

electrostat commented Jul 18, 2018

tobrun commented Jul 19, 2018 •

edited

Loading

LukasPaczos commented Jul 19, 2018

jaegs Jul 19, 2018

jaegs Jul 19, 2018

tobrun Jul 19, 2018

jaegs Jul 19, 2018

tobrun commented Jul 20, 2018


		public abstract void cancelRequest(long nativePtr);

		public static void execute(NativeHttpRequest httpRequest, long nativePtr, String resourceUrl,

HttpClient configuration #12423

HttpClient configuration #12423

Conversation

tobrun commented Jul 18, 2018

zugaldia commented Jul 18, 2018

LukasPaczos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrostat commented Jul 18, 2018

tobrun commented Jul 19, 2018 • edited Loading

LukasPaczos commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun commented Jul 20, 2018

tobrun commented Jul 19, 2018 •

edited

Loading