Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

HttpClient configuration #12423

Closed
wants to merge 1 commit into from
Closed

HttpClient configuration #12423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions platform/android/MapboxGLAndroidSDK/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ dependencies {
}
implementation dependenciesList.supportAnnotations
implementation dependenciesList.supportFragmentV4
implementation dependenciesList.supportUtilV4
implementation dependenciesList.timber
implementation dependenciesList.okhttp3
testImplementation dependenciesList.junit
Expand Down
3 changes: 2 additions & 1 deletion platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.mapbox.mapboxsdk.exceptions.MapboxConfigurationException;
import com.mapbox.mapboxsdk.maps.Telemetry;
import com.mapbox.mapboxsdk.net.ConnectivityReceiver;

import timber.log.Timber;

/**
Expand Down Expand Up @@ -47,7 +48,7 @@ public static synchronized Mapbox getInstance(@NonNull Context context, @Nullabl
Context appContext = context.getApplicationContext();
INSTANCE = new Mapbox(appContext, accessToken);
if (isAccessTokenValid(accessToken)) {
initializeTelemetry();
//initializeTelemetry();
}
ConnectivityReceiver.instance(appContext);
}
Expand Down
273 changes: 0 additions & 273 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java
View file
Open in desktop

This file was deleted.

37 changes: 37 additions & 0 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.mapbox.mapboxsdk.http;

import android.support.annotation.Nullable;

public abstract class HttpRequest {

private static HttpRequest httpRequest;

static final int CONNECTION_ERROR = 0;
static final int TEMPORARY_ERROR = 1;
static final int PERMANENT_ERROR = 2;

public static synchronized void setHttpRequest(@Nullable HttpRequest requestImpl) {
httpRequest = requestImpl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

public abstract void executeRequest(NativeHttpRequest httpRequest, long nativePtr, String resourceUrl,
String etag, String modified);

public abstract void cancelRequest(long nativePtr);

static void execute(NativeHttpRequest httpRequest, long nativePtr, String resourceUrl,
String etag, String modified) {
getInstance().executeRequest(httpRequest, nativePtr, resourceUrl, etag, modified);
}

static void cancel(long nativePtr) {
getInstance().cancelRequest(nativePtr);
}

private static synchronized HttpRequest getInstance() {
if (httpRequest == null) {
httpRequest = new OkHttpRequest();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return httpRequest;
}
}
10 changes: 5 additions & 5 deletions ...m/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@
import okhttp3.OkHttpClient;

/**
* Utility class for setting HttpRequest configurations
* Utility class for setting OkHttpRequest configurations
*/
public class HttpRequestUtil {

/**
* Set the log state of HttpRequest. Default value is true.
* Set the log state of OkHttpRequest. Default value is true.
* <p>
* This configuration will outlast the lifecycle of the Map.
* </p>
*
* @param enabled True will enable logging, false will disable
*/
public static void setLogEnabled(boolean enabled) {
HTTPRequest.enableLog(enabled);
OkHttpRequest.enableLog(enabled);
}

/**
Expand All @@ -31,7 +31,7 @@ public static void setLogEnabled(boolean enabled) {
* @param enabled True will print urls, false will disable
*/
public static void setPrintRequestUrlOnFailure(boolean enabled) {
HTTPRequest.enablePrintRequestUrlOnFailure(enabled);
OkHttpRequest.enablePrintRequestUrlOnFailure(enabled);
}

/**
Expand All @@ -40,7 +40,7 @@ public static void setPrintRequestUrlOnFailure(boolean enabled) {
* @param client the OkHttpClient
*/
public static void setOkHttpClient(OkHttpClient client) {
HTTPRequest.setOKHttpClient(client);
OkHttpRequest.setOkHttpClient(client);
}

}
Loading