From efe8023aed474e4bf1a596faf15ca1e4c5f9e34f Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Fri, 20 Mar 2020 16:14:37 -0700 Subject: [PATCH] Try to more reliably include http status codes in HttpUrlFetcher exceptions. PiperOrigin-RevId: 302119882 --- .../cronet/ChromiumUrlFetcherTest.java | 2 +- .../bumptech/glide/load/HttpException.java | 9 +- .../glide/load/data/HttpUrlFetcher.java | 109 +++++++++++++----- .../glide/load/data/HttpUrlFetcherTest.java | 74 +++++++++++- 4 files changed, 155 insertions(+), 39 deletions(-) diff --git a/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java b/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java index bd47e9d875..ee67f49c6b 100644 --- a/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java +++ b/integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java @@ -299,7 +299,7 @@ public void testRequestComplete_withNon200StatusCode_callsCallbackWithException( verify(callback, timeout(1000)).onLoadFailed(captor.capture()); assertThat(captor.getValue()) .hasMessageThat() - .isEqualTo("Http request failed with status code: 500"); + .isEqualTo("Http request failed, status code: 500"); } private void succeed(UrlResponseInfo info, Callback urlCallback, ByteBuffer byteBuffer) diff --git a/library/src/main/java/com/bumptech/glide/load/HttpException.java b/library/src/main/java/com/bumptech/glide/load/HttpException.java index ce265c6c12..35e735d289 100644 --- a/library/src/main/java/com/bumptech/glide/load/HttpException.java +++ b/library/src/main/java/com/bumptech/glide/load/HttpException.java @@ -19,9 +19,14 @@ public final class HttpException extends IOException { private final int statusCode; public HttpException(int statusCode) { - this("Http request failed with status code: " + statusCode, statusCode); + this("Http request failed", statusCode); } + /** + * @deprecated You should always include a status code, default to {@link #UNKNOWN} if you can't + * come up with a reasonable one. This method will be removed in a future version. + */ + @Deprecated public HttpException(String message) { this(message, UNKNOWN); } @@ -31,7 +36,7 @@ public HttpException(String message, int statusCode) { } public HttpException(String message, int statusCode, @Nullable Throwable cause) { - super(message, cause); + super(message + ", status code: " + statusCode, cause); this.statusCode = statusCode; } diff --git a/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java b/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java index 922a9c6b77..5602c1498c 100644 --- a/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java +++ b/library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java @@ -14,6 +14,7 @@ import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; +import java.net.MalformedURLException; import java.net.URISyntaxException; import java.net.URL; import java.util.Map; @@ -22,12 +23,13 @@ public class HttpUrlFetcher implements DataFetcher { private static final String TAG = "HttpUrlFetcher"; private static final int MAXIMUM_REDIRECTS = 5; + @VisibleForTesting static final String REDIRECT_HEADER_FIELD = "Location"; @VisibleForTesting static final HttpUrlConnectionFactory DEFAULT_CONNECTION_FACTORY = new DefaultHttpUrlConnectionFactory(); /** Returned when a connection error prevented us from receiving an http error. */ - private static final int INVALID_STATUS_CODE = -1; + @VisibleForTesting static final int INVALID_STATUS_CODE = -1; private final GlideUrl glideUrl; private final int timeout; @@ -68,50 +70,52 @@ public void loadData( } private InputStream loadDataWithRedirects( - URL url, int redirects, URL lastUrl, Map headers) throws IOException { + URL url, int redirects, URL lastUrl, Map headers) throws HttpException { if (redirects >= MAXIMUM_REDIRECTS) { - throw new HttpException("Too many (> " + MAXIMUM_REDIRECTS + ") redirects!"); + throw new HttpException( + "Too many (> " + MAXIMUM_REDIRECTS + ") redirects!", INVALID_STATUS_CODE); } else { // Comparing the URLs using .equals performs additional network I/O and is generally broken. // See http://michaelscharf.blogspot.com/2006/11/javaneturlequals-and-hashcode-make.html. try { if (lastUrl != null && url.toURI().equals(lastUrl.toURI())) { - throw new HttpException("In re-direct loop"); + throw new HttpException("In re-direct loop", INVALID_STATUS_CODE); } } catch (URISyntaxException e) { // Do nothing, this is best effort. } } - urlConnection = connectionFactory.build(url); - for (Map.Entry headerEntry : headers.entrySet()) { - urlConnection.addRequestProperty(headerEntry.getKey(), headerEntry.getValue()); - } - urlConnection.setConnectTimeout(timeout); - urlConnection.setReadTimeout(timeout); - urlConnection.setUseCaches(false); - urlConnection.setDoInput(true); + urlConnection = buildAndConfigureConnection(url, headers); - // Stop the urlConnection instance of HttpUrlConnection from following redirects so that - // redirects will be handled by recursive calls to this method, loadDataWithRedirects. - urlConnection.setInstanceFollowRedirects(false); + try { + // Connect explicitly to avoid errors in decoders if connection fails. + urlConnection.connect(); + // Set the stream so that it's closed in cleanup to avoid resource leaks. See #2352. + stream = urlConnection.getInputStream(); + } catch (IOException e) { + throw new HttpException( + "Failed to connect or obtain data", getHttpStatusCodeOrInvalid(urlConnection), e); + } - // Connect explicitly to avoid errors in decoders if connection fails. - urlConnection.connect(); - // Set the stream so that it's closed in cleanup to avoid resource leaks. See #2352. - stream = urlConnection.getInputStream(); if (isCancelled) { return null; } - final int statusCode = urlConnection.getResponseCode(); + + final int statusCode = getHttpStatusCodeOrInvalid(urlConnection); if (isHttpOk(statusCode)) { return getStreamForSuccessfulRequest(urlConnection); } else if (isHttpRedirect(statusCode)) { - String redirectUrlString = urlConnection.getHeaderField("Location"); + String redirectUrlString = urlConnection.getHeaderField(REDIRECT_HEADER_FIELD); if (TextUtils.isEmpty(redirectUrlString)) { - throw new HttpException("Received empty or null redirect url"); + throw new HttpException("Received empty or null redirect url", statusCode); + } + URL redirectUrl; + try { + redirectUrl = new URL(url, redirectUrlString); + } catch (MalformedURLException e) { + throw new HttpException("Bad redirect url: " + redirectUrlString, statusCode, e); } - URL redirectUrl = new URL(url, redirectUrlString); // Closing the stream specifically is required to avoid leaking ResponseBodys in addition // to disconnecting the url connection below. See #2352. cleanup(); @@ -119,8 +123,44 @@ private InputStream loadDataWithRedirects( } else if (statusCode == INVALID_STATUS_CODE) { throw new HttpException(statusCode); } else { - throw new HttpException(urlConnection.getResponseMessage(), statusCode); + try { + throw new HttpException(urlConnection.getResponseMessage(), statusCode); + } catch (IOException e) { + throw new HttpException("Failed to get a response message", statusCode, e); + } + } + } + + private static int getHttpStatusCodeOrInvalid(HttpURLConnection urlConnection) { + try { + return urlConnection.getResponseCode(); + } catch (IOException e) { + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "Failed to get a response code", e); + } + } + return INVALID_STATUS_CODE; + } + + private HttpURLConnection buildAndConfigureConnection(URL url, Map headers) + throws HttpException { + HttpURLConnection urlConnection; + try { + urlConnection = connectionFactory.build(url); + } catch (IOException e) { + throw new HttpException("URL.openConnection threw", /*statusCode=*/ 0, e); + } + for (Map.Entry headerEntry : headers.entrySet()) { + urlConnection.addRequestProperty(headerEntry.getKey(), headerEntry.getValue()); } + urlConnection.setConnectTimeout(timeout); + urlConnection.setReadTimeout(timeout); + urlConnection.setUseCaches(false); + urlConnection.setDoInput(true); + // Stop the urlConnection instance of HttpUrlConnection from following redirects so that + // redirects will be handled by recursive calls to this method, loadDataWithRedirects. + urlConnection.setInstanceFollowRedirects(false); + return urlConnection; } // Referencing constants is less clear than a simple static method. @@ -134,15 +174,20 @@ private static boolean isHttpRedirect(int statusCode) { } private InputStream getStreamForSuccessfulRequest(HttpURLConnection urlConnection) - throws IOException { - if (TextUtils.isEmpty(urlConnection.getContentEncoding())) { - int contentLength = urlConnection.getContentLength(); - stream = ContentLengthInputStream.obtain(urlConnection.getInputStream(), contentLength); - } else { - if (Log.isLoggable(TAG, Log.DEBUG)) { - Log.d(TAG, "Got non empty content encoding: " + urlConnection.getContentEncoding()); + throws HttpException { + try { + if (TextUtils.isEmpty(urlConnection.getContentEncoding())) { + int contentLength = urlConnection.getContentLength(); + stream = ContentLengthInputStream.obtain(urlConnection.getInputStream(), contentLength); + } else { + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "Got non empty content encoding: " + urlConnection.getContentEncoding()); + } + stream = urlConnection.getInputStream(); } - stream = urlConnection.getInputStream(); + } catch (IOException e) { + throw new HttpException( + "Failed to obtain InputStream", getHttpStatusCodeOrInvalid(urlConnection), e); } return stream; } diff --git a/library/test/src/test/java/com/bumptech/glide/load/data/HttpUrlFetcherTest.java b/library/test/src/test/java/com/bumptech/glide/load/data/HttpUrlFetcherTest.java index f3c92cf5a8..a471dc4534 100644 --- a/library/test/src/test/java/com/bumptech/glide/load/data/HttpUrlFetcherTest.java +++ b/library/test/src/test/java/com/bumptech/glide/load/data/HttpUrlFetcherTest.java @@ -1,22 +1,28 @@ package com.bumptech.glide.load.data; +import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import com.bumptech.glide.Priority; +import com.bumptech.glide.load.HttpException; import com.bumptech.glide.load.model.GlideUrl; import java.io.ByteArrayInputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.net.HttpURLConnection; +import java.net.MalformedURLException; import java.net.URL; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -49,13 +55,73 @@ public void setUp() throws IOException { } @Test - public void testSetsReadTimeout() throws IOException { + public void loadData_whenConnectThrowsFileNotFound_notifiesCallbackWithHttpErrorCode() + throws IOException { + int statusCode = 400; + doThrow(new FileNotFoundException()).when(urlConnection).connect(); + when(urlConnection.getResponseCode()).thenReturn(statusCode); + + fetcher.loadData(Priority.HIGH, callback); + + HttpException exception = (HttpException) getCallbackException(); + assertThat(exception.getStatusCode()).isEqualTo(statusCode); + } + + @Test + public void loadData_whenGetInputStreamThrows_notifiesCallbackWithStatusCode() + throws IOException { + int statusCode = 400; + doThrow(new IOException()).when(urlConnection).getInputStream(); + when(urlConnection.getResponseCode()).thenReturn(statusCode); + + fetcher.loadData(Priority.HIGH, callback); + + HttpException exception = (HttpException) getCallbackException(); + assertThat(exception.getStatusCode()).isEqualTo(statusCode); + } + + @Test + public void loadData_whenConnectAndGetResponseCodeThrow_notifiesCallbackWithInvalidStatusCode() + throws IOException { + doThrow(new FileNotFoundException()).when(urlConnection).connect(); + when(urlConnection.getResponseCode()).thenThrow(new IOException()); + + fetcher.loadData(Priority.HIGH, callback); + + HttpException exception = (HttpException) getCallbackException(); + assertThat(exception.getStatusCode()).isEqualTo(HttpUrlFetcher.INVALID_STATUS_CODE); + } + + @Test + public void loadData_whenRedirectUrlIsMalformed_notifiesCallbackWithStatusCode() + throws IOException { + int statusCode = 300; + + when(urlConnection.getHeaderField(eq(HttpUrlFetcher.REDIRECT_HEADER_FIELD))) + .thenReturn("gg://www.google.com"); + when(urlConnection.getResponseCode()).thenReturn(statusCode); + + fetcher.loadData(Priority.HIGH, callback); + + HttpException exception = (HttpException) getCallbackException(); + assertThat(exception.getStatusCode()).isEqualTo(statusCode); + assertThat(exception.getCause()).isInstanceOf(MalformedURLException.class); + } + + private Exception getCallbackException() { + ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); + verify(callback).onLoadFailed(captor.capture()); + return captor.getValue(); + } + + @Test + public void testSetsReadTimeout() { fetcher.loadData(Priority.HIGH, callback); verify(urlConnection).setReadTimeout(eq(TIMEOUT_MS)); } @Test - public void testSetsConnectTimeout() throws IOException { + public void testSetsConnectTimeout() { fetcher.loadData(Priority.IMMEDIATE, callback); verify(urlConnection).setConnectTimeout(eq(TIMEOUT_MS)); } @@ -71,7 +137,7 @@ public void testReturnsNullIfCancelledBeforeConnects() throws IOException { } @Test - public void testDisconnectsUrlOnCleanup() throws IOException { + public void testDisconnectsUrlOnCleanup() { fetcher.loadData(Priority.HIGH, callback); fetcher.cleanup(); @@ -89,7 +155,7 @@ public void testDoesNotThrowIfCancelCalledBeforeStart() { } @Test - public void testCancelDoesNotDisconnectIfAlreadyConnected() throws IOException { + public void testCancelDoesNotDisconnectIfAlreadyConnected() { fetcher.loadData(Priority.HIGH, callback); fetcher.cancel();