[android] - check for null body on http request, cleanup code

Author: tobrun

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

@@ -4,7 +4,9 @@
 import android.content.Context;
 import android.content.pm.PackageInfo;
 import android.os.Build;
+import android.support.annotation.NonNull;
 import android.text.TextUtils;
+import android.util.Log;
 
 import com.mapbox.mapboxsdk.BuildConfig;
 import com.mapbox.mapboxsdk.Mapbox;
@@ -27,54 +29,42 @@
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.Response;
+import okhttp3.ResponseBody;
 import okhttp3.internal.Util;
 import timber.log.Timber;
 
 import static android.util.Log.DEBUG;
+import static android.util.Log.ERROR;
 import static android.util.Log.INFO;
+import static android.util.Log.VERBOSE;
 import static android.util.Log.WARN;
 
 class HTTPRequest implements Callback {
 
-  private static OkHttpClient mClient = new OkHttpClient.Builder().dispatcher(getDispatcher()).build();
-  private static boolean logEnabled = true;
-  private static boolean logRequestUrl = false;
-
-  private String USER_AGENT_STRING = null;
-
   private static final int CONNECTION_ERROR = 0;
   private static final int TEMPORARY_ERROR = 1;
   private static final int PERMANENT_ERROR = 2;
 
-  // Reentrancy is not needed, but "Lock" is an
-  // abstract class.
-  private ReentrantLock mLock = new ReentrantLock();
-
-  private long mNativePtr = 0;
-
-  private Call mCall;
-  private Request mRequest;
-
-  private static Dispatcher getDispatcher() {
-    Dispatcher dispatcher = new Dispatcher();
-    // Matches core limit set on
-    // https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/src/http_file_source.cpp#L192
-    dispatcher.setMaxRequestsPerHost(20);
-    return dispatcher;
-  }
-
-  private native void nativeOnFailure(int type, String message);
+  private static OkHttpClient client = new OkHttpClient.Builder().dispatcher(getDispatcher()).build();
+  private static boolean logEnabled = true;
+  private static boolean logRequestUrl = false;
 
-  private native void nativeOnResponse(int code, String etag, String modified, String cacheControl, String expires,
-                                       String retryAfter, String xRateLimitReset, byte[] body);
+  // Reentrancy is not needed, but "Lock" is an abstract class.
+  private ReentrantLock lock = new ReentrantLock();
+  private String userAgentString;
+  private long nativePtr = 0;
+  private Call call;
 
   private HTTPRequest(long nativePtr, String resourceUrl, String etag, String modified) {
-    mNativePtr = nativePtr;
+    this.nativePtr = nativePtr;
 
     try {
       HttpUrl httpUrl = HttpUrl.parse(resourceUrl);
-      final String host = httpUrl.host().toLowerCase(MapboxConstants.MAPBOX_LOCALE);
+      if (httpUrl == null) {
+        log(Log.ERROR, String.format("[HTTP] Unable to parse resourceUrl %s", resourceUrl));
+      }
 
+      final String host = httpUrl.host().toLowerCase(MapboxConstants.MAPBOX_LOCALE);
       // Don't try a request to remote server if we aren't connected
       if (!Mapbox.isConnected() && !host.equals("127.0.0.1") && !host.equals("localhost")) {
         throw new NoRouteToHostException("No Internet connection available.");
@@ -99,56 +89,59 @@ private HTTPRequest(long nativePtr, String resourceUrl, String etag, String modi
       } else if (modified.length() > 0) {
         builder = builder.addHeader("If-Modified-Since", modified);
       }
-      mRequest = builder.build();
-      mCall = mClient.newCall(mRequest);
-      mCall.enqueue(this);
+      Request request = builder.build();
+      call = client.newCall(request);
+      call.enqueue(this);
     } catch (Exception exception) {
-      handleFailure(mCall, exception);
+      handleFailure(call, exception);
     }
   }
 
   public void cancel() {
-    // mCall can be null if the constructor gets aborted (e.g, under a NoRouteToHostException).
-    if (mCall != null) {
-      mCall.cancel();
+    // call can be null if the constructor gets aborted (e.g, under a NoRouteToHostException).
+    if (call != null) {
+      call.cancel();
     }
 
     // TODO: We need a lock here because we can try
     // to cancel at the same time the request is getting
     // answered on the OkHTTP thread. We could get rid of
     // this lock by using Runnable when we move Android
     // implementation of mbgl::RunLoop to Looper.
-    mLock.lock();
-    mNativePtr = 0;
-    mLock.unlock();
+    lock.lock();
+    nativePtr = 0;
+    lock.unlock();
   }
 
   @Override
-  public void onResponse(Call call, Response response) throws IOException {
+  public void onResponse(@NonNull Call call, @NonNull Response response) throws IOException {
+    if (response.isSuccessful()) {
+      log(VERBOSE, String.format("[HTTP] Request was successful (code = %s).", response.code()));
+    } else {
+      // We don't want to call this unsuccessful because a 304 isn't really an error
+      String message = !TextUtils.isEmpty(response.message()) ? response.message() : "No additional information";
+      log(DEBUG, String.format("[HTTP] Request with response code = %s: %s", response.code(), message));
+    }
 
-    if (logEnabled) {
-      if (response.isSuccessful()) {
-        Timber.v("[HTTP] Request was successful (code = %s).", response.code());
-      } else {
-        // We don't want to call this unsuccessful because a 304 isn't really an error
-        String message = !TextUtils.isEmpty(response.message()) ? response.message() : "No additional information";
-        Timber.d("[HTTP] Request with response code = %s: %s", response.code(), message);
-      }
+    ResponseBody responseBody = response.body();
+    if (responseBody == null) {
+      log(ERROR, "[HTTP] Received empty response body");
+      return;
     }
 
     byte[] body;
     try {
-      body = response.body().bytes();
+      body = responseBody.bytes();
     } catch (IOException ioException) {
       onFailure(call, ioException);
       // throw ioException;
       return;
     } finally {
-      response.body().close();
+      response.close();
     }
 
-    mLock.lock();
-    if (mNativePtr != 0) {
+    lock.lock();
+    if (nativePtr != 0) {
       nativeOnResponse(response.code(),
         response.header("ETag"),
         response.header("Last-Modified"),
@@ -158,14 +151,34 @@ public void onResponse(Call call, Response response) throws IOException {
         response.header("x-rate-limit-reset"),
         body);
     }
-    mLock.unlock();
+    lock.unlock();
   }
 
   @Override
-  public void onFailure(Call call, IOException e) {
+  public void onFailure(@NonNull Call call, @NonNull IOException e) {
     handleFailure(call, e);
   }
 
+  static void enableLog(boolean enabled) {
+    logEnabled = enabled;
+  }
+
+  static void enablePrintRequestUrlOnFailure(boolean enabled) {
+    logRequestUrl = enabled;
+  }
+
+  static void setOKHttpClient(OkHttpClient client) {
+    HTTPRequest.client = client;
+  }
+
+  private static Dispatcher getDispatcher() {
+    Dispatcher dispatcher = new Dispatcher();
+    // Matches core limit set on
+    // https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/src/http_file_source.cpp#L192
+    dispatcher.setMaxRequestsPerHost(20);
+    return dispatcher;
+  }
+
   private void handleFailure(Call call, Exception e) {
     String errorMessage = e.getMessage() != null ? e.getMessage() : "Error processing the request";
     int type = getFailureType(e);
@@ -175,11 +188,11 @@ private void handleFailure(Call call, Exception e) {
       logFailure(type, errorMessage, requestUrl);
     }
 
-    mLock.lock();
-    if (mNativePtr != 0) {
+    lock.lock();
+    if (nativePtr != 0) {
       nativeOnFailure(type, errorMessage);
     }
-    mLock.unlock();
+    lock.unlock();
   }
 
   private int getFailureType(Exception e) {
@@ -192,29 +205,35 @@ private int getFailureType(Exception e) {
     return PERMANENT_ERROR;
   }
 
+  private void log(int type, String errorMessage) {
+    if (logEnabled) {
+      Timber.log(type, errorMessage);
+    }
+  }
+
   private void logFailure(int type, String errorMessage, String requestUrl) {
-    Timber.log(
-      type == TEMPORARY_ERROR ? DEBUG : type == CONNECTION_ERROR ? INFO : WARN,
-      "Request failed due to a %s error: %s %s",
-      type == TEMPORARY_ERROR ? "temporary" : type == CONNECTION_ERROR ? "connection" : "permanent",
-      errorMessage,
-      logRequestUrl ? requestUrl : ""
+    log(type == TEMPORARY_ERROR ? DEBUG : type == CONNECTION_ERROR ? INFO : WARN,
+      String.format(
+        "Request failed due to a %s error: %s %s",
+        type == TEMPORARY_ERROR ? "temporary" : type == CONNECTION_ERROR ? "connection" : "permanent",
+        errorMessage,
+        logRequestUrl ? requestUrl : ""
+      )
     );
   }
 
   private String getUserAgent() {
-    if (USER_AGENT_STRING == null) {
-      return USER_AGENT_STRING = Util.toHumanReadableAscii(
+    if (userAgentString == null) {
+      userAgentString = Util.toHumanReadableAscii(
         String.format("%s %s (%s) Android/%s (%s)",
           getApplicationIdentifier(),
           BuildConfig.MAPBOX_VERSION_STRING,
           BuildConfig.GIT_REVISION_SHORT,
           Build.VERSION.SDK_INT,
           Build.CPU_ABI)
       );
-    } else {
-      return USER_AGENT_STRING;
     }
+    return userAgentString;
   }
 
   private String getApplicationIdentifier() {
@@ -227,15 +246,8 @@ private String getApplicationIdentifier() {
     }
   }
 
-  static void enableLog(boolean enabled) {
-    logEnabled = enabled;
-  }
-
-  static void enablePrintRequestUrlOnFailure(boolean enabled) {
-    logRequestUrl = enabled;
-  }
+  private native void nativeOnFailure(int type, String message);
 
-  static void setOKHttpClient(OkHttpClient client) {
-    mClient = client;
-  }
+  private native void nativeOnResponse(int code, String etag, String modified, String cacheControl, String expires,
+                                       String retryAfter, String xRateLimitReset, byte[] body);
 }

platform/android/src/http_file_source.cpp

@@ -61,7 +61,7 @@ void RegisterNativeHTTPRequest(jni::JNIEnv& env) {
 
     #define METHOD(MethodPtr, name) jni::MakeNativePeerMethod<decltype(MethodPtr), (MethodPtr)>(name)
 
-    jni::RegisterNativePeer<HTTPRequest>(env, HTTPRequest::javaClass, "mNativePtr",
+    jni::RegisterNativePeer<HTTPRequest>(env, HTTPRequest::javaClass, "nativePtr",
         METHOD(&HTTPRequest::onFailure, "nativeOnFailure"),
         METHOD(&HTTPRequest::onResponse, "nativeOnResponse"));
 }