Try to more reliably include http status codes in HttpUrlFetcher exce…

…ptions.

PiperOrigin-RevId: 302119882

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)

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;
   }
 

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<InputStream> {
   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,59 +70,97 @@ public void loadData(
   }
 
   private InputStream loadDataWithRedirects(
-      URL url, int redirects, URL lastUrl, Map<String, String> headers) throws IOException {
+      URL url, int redirects, URL lastUrl, Map<String, String> 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<String, String> 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();
       return loadDataWithRedirects(redirectUrl, redirects + 1, url, headers);
     } 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<String, String> 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<String, String> 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;
   }

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<Exception> 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();