From e3071ab057644b92dc49529bc5783947fe7cb744 Mon Sep 17 00:00:00 2001 From: Tobrun Date: Thu, 8 Mar 2018 07:30:31 +0100 Subject: [android] - check for null body on http request, cleanup code --- .../com/mapbox/mapboxsdk/http/HTTPRequest.java | 164 +++++++++++---------- platform/android/src/http_file_source.cpp | 2 +- 2 files changed, 89 insertions(+), 77 deletions(-) diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java index caee493e6f..ffc55fec46 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HTTPRequest.java @@ -4,7 +4,9 @@ package com.mapbox.mapboxsdk.http; 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.HttpUrl; 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,18 +89,18 @@ class HTTPRequest implements Callback { } 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 @@ -118,37 +108,40 @@ class HTTPRequest implements Callback { // 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 @@ class HTTPRequest implements Callback { 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 @@ class HTTPRequest implements Callback { 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,19 +205,26 @@ class HTTPRequest implements Callback { 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, @@ -212,9 +232,8 @@ class HTTPRequest implements Callback { Build.VERSION.SDK_INT, Build.CPU_ABI) ); - } else { - return USER_AGENT_STRING; } + return userAgentString; } private String getApplicationIdentifier() { @@ -227,15 +246,8 @@ class HTTPRequest implements Callback { } } - 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); } diff --git a/platform/android/src/http_file_source.cpp b/platform/android/src/http_file_source.cpp index 8eb9416e9d..cda84209ea 100644 --- a/platform/android/src/http_file_source.cpp +++ b/platform/android/src/http_file_source.cpp @@ -61,7 +61,7 @@ void RegisterNativeHTTPRequest(jni::JNIEnv& env) { #define METHOD(MethodPtr, name) jni::MakeNativePeerMethod(name) - jni::RegisterNativePeer(env, HTTPRequest::javaClass, "mNativePtr", + jni::RegisterNativePeer(env, HTTPRequest::javaClass, "nativePtr", METHOD(&HTTPRequest::onFailure, "nativeOnFailure"), METHOD(&HTTPRequest::onResponse, "nativeOnResponse")); } -- cgit v1.2.1