From 0e0c3d9cfa694f8f1400a9e9abc4bc11761fdb52 Mon Sep 17 00:00:00 2001 From: Jeff Davidson Date: Wed, 3 Feb 2021 15:06:52 -0800 Subject: [PATCH] Invoke RetryPolicy#retry in the blocking executor. (#393) When using the synchronous RequestQueue, retry will be invoked in the NetworkDispatcher thread. Clients may be relying on the ability to perform blocking operations in retry(); a common example would be clearing auth tokens in the event of an auth error, which may require disk I/O. In order to keep the same API contract when using AsyncRequestQueue, rework NetworkUtility to permit BasicAsyncNetwork to invoke retry in the blocking executor. --- .../volley/toolbox/BasicAsyncNetwork.java | 35 +++++++++-- .../android/volley/toolbox/BasicNetwork.java | 8 ++- .../volley/toolbox/NetworkUtility.java | 60 +++++++++++-------- 3 files changed, 72 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/android/volley/toolbox/BasicAsyncNetwork.java b/src/main/java/com/android/volley/toolbox/BasicAsyncNetwork.java index 55892a03..15590eae 100644 --- a/src/main/java/com/android/volley/toolbox/BasicAsyncNetwork.java +++ b/src/main/java/com/android/volley/toolbox/BasicAsyncNetwork.java @@ -29,6 +29,7 @@ import com.android.volley.Request; import com.android.volley.RequestTask; import com.android.volley.VolleyError; +import com.android.volley.toolbox.NetworkUtility.RetryInfo; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; @@ -126,13 +127,39 @@ private void onRequestFailed( @Nullable HttpResponse httpResponse, @Nullable byte[] responseContents) { try { - NetworkUtility.handleException( - request, exception, requestStartMs, httpResponse, responseContents); + RetryInfo retryInfo = + NetworkUtility.shouldRetryException( + request, exception, requestStartMs, httpResponse, responseContents); + // RetryPolicy#retry may need a background thread, so invoke in the blocking executor. + getBlockingExecutor() + .execute(new InvokeRetryPolicyTask<>(request, retryInfo, callback)); } catch (VolleyError volleyError) { callback.onError(volleyError); - return; } - performRequest(request, callback); + } + + private class InvokeRetryPolicyTask extends RequestTask { + final Request request; + final RetryInfo retryInfo; + final OnRequestComplete callback; + + InvokeRetryPolicyTask(Request request, RetryInfo retryInfo, OnRequestComplete callback) { + super(request); + this.request = request; + this.retryInfo = retryInfo; + this.callback = callback; + } + + @Override + public void run() { + try { + NetworkUtility.attemptRetryOnException(request, retryInfo); + // attemptRetryOnException didn't throw, so proceed with the next attempt. + performRequest(request, callback); + } catch (VolleyError e) { + callback.onError(e); + } + } } @Override diff --git a/src/main/java/com/android/volley/toolbox/BasicNetwork.java b/src/main/java/com/android/volley/toolbox/BasicNetwork.java index 06427fe0..552e6286 100644 --- a/src/main/java/com/android/volley/toolbox/BasicNetwork.java +++ b/src/main/java/com/android/volley/toolbox/BasicNetwork.java @@ -22,6 +22,7 @@ import com.android.volley.NetworkResponse; import com.android.volley.Request; import com.android.volley.VolleyError; +import com.android.volley.toolbox.NetworkUtility.RetryInfo; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; @@ -140,8 +141,11 @@ public NetworkResponse performRequest(Request request) throws VolleyError { } catch (IOException e) { // This will either throw an exception, breaking us from the loop, or will loop // again and retry the request. - NetworkUtility.handleException( - request, e, requestStart, httpResponse, responseContents); + RetryInfo retryInfo = + NetworkUtility.shouldRetryException( + request, e, requestStart, httpResponse, responseContents); + // We should already be on a background thread, so we can invoke the retry inline. + NetworkUtility.attemptRetryOnException(request, retryInfo); } } } diff --git a/src/main/java/com/android/volley/toolbox/NetworkUtility.java b/src/main/java/com/android/volley/toolbox/NetworkUtility.java index 44d59043..3bdd3dc6 100644 --- a/src/main/java/com/android/volley/toolbox/NetworkUtility.java +++ b/src/main/java/com/android/volley/toolbox/NetworkUtility.java @@ -113,30 +113,45 @@ static byte[] inputStreamToBytes(InputStream in, int contentLength, ByteArrayPoo /** * Attempts to prepare the request for a retry. If there are no more attempts remaining in the - * request's retry policy, a timeout exception is thrown. + * request's retry policy, the provided exception is thrown. + * + *

Must be invoked from a background thread, as client implementations of RetryPolicy#retry + * may make blocking calls. * * @param request The request to use. */ - private static void attemptRetryOnException( - final String logPrefix, final Request request, final VolleyError exception) + static void attemptRetryOnException(final Request request, final RetryInfo retryInfo) throws VolleyError { final RetryPolicy retryPolicy = request.getRetryPolicy(); final int oldTimeout = request.getTimeoutMs(); try { - retryPolicy.retry(exception); + retryPolicy.retry(retryInfo.errorToRetry); } catch (VolleyError e) { request.addMarker( - String.format("%s-timeout-giveup [timeout=%s]", logPrefix, oldTimeout)); + String.format( + "%s-timeout-giveup [timeout=%s]", retryInfo.logPrefix, oldTimeout)); throw e; } - request.addMarker(String.format("%s-retry [timeout=%s]", logPrefix, oldTimeout)); + request.addMarker(String.format("%s-retry [timeout=%s]", retryInfo.logPrefix, oldTimeout)); + } + + static class RetryInfo { + private final String logPrefix; + private final VolleyError errorToRetry; + + private RetryInfo(String logPrefix, VolleyError errorToRetry) { + this.logPrefix = logPrefix; + this.errorToRetry = errorToRetry; + } } /** * Based on the exception thrown, decides whether to attempt to retry, or to throw the error. - * Also handles logging. + * + *

If this method returns without throwing, {@link #attemptRetryOnException} should be called + * with the provided {@link RetryInfo} to consult the client's retry policy. */ - static void handleException( + static RetryInfo shouldRetryException( Request request, IOException exception, long requestStartMs, @@ -144,7 +159,7 @@ static void handleException( @Nullable byte[] responseContents) throws VolleyError { if (exception instanceof SocketTimeoutException) { - attemptRetryOnException("socket", request, new TimeoutError()); + return new RetryInfo("socket", new TimeoutError()); } else if (exception instanceof MalformedURLException) { throw new RuntimeException("Bad URL " + request.getUrl(), exception); } else { @@ -153,11 +168,9 @@ static void handleException( statusCode = httpResponse.getStatusCode(); } else { if (request.shouldRetryConnectionErrors()) { - attemptRetryOnException("connection", request, new NoConnectionError()); - return; - } else { - throw new NoConnectionError(exception); + return new RetryInfo("connection", new NoConnectionError()); } + throw new NoConnectionError(exception); } VolleyLog.e("Unexpected response code %d for %s", statusCode, request.getUrl()); NetworkResponse networkResponse; @@ -173,24 +186,21 @@ static void handleException( responseHeaders); if (statusCode == HttpURLConnection.HTTP_UNAUTHORIZED || statusCode == HttpURLConnection.HTTP_FORBIDDEN) { - attemptRetryOnException("auth", request, new AuthFailureError(networkResponse)); - } else if (statusCode >= 400 && statusCode <= 499) { + return new RetryInfo("auth", new AuthFailureError(networkResponse)); + } + if (statusCode >= 400 && statusCode <= 499) { // Don't retry other client errors. throw new ClientError(networkResponse); - } else if (statusCode >= 500 && statusCode <= 599) { + } + if (statusCode >= 500 && statusCode <= 599) { if (request.shouldRetryServerErrors()) { - attemptRetryOnException( - "server", request, new ServerError(networkResponse)); - } else { - throw new ServerError(networkResponse); + return new RetryInfo("server", new ServerError(networkResponse)); } - } else { - // 3xx? No reason to retry. - throw new ServerError(networkResponse); } - } else { - attemptRetryOnException("network", request, new NetworkError()); + // Server error and client has opted out of retries, or 3xx. No reason to retry. + throw new ServerError(networkResponse); } + return new RetryInfo("network", new NetworkError()); } } }