Skip to content

Commit efe8023

Browse files
sjuddglide-copybara-robot
sjudd
authored and
glide-copybara-robot
committedMar 20, 2020
Try to more reliably include http status codes in HttpUrlFetcher exceptions.
PiperOrigin-RevId: 302119882
·
v5.0.0-rc01compose-alpha6
1 parent 3d8bf6b commit efe8023

File tree

4 files changed

+155
-39
lines changed

4 files changed

+155
-39
lines changed
 

‎integration/cronet/src/test/java/com/bumptech/glide/integration/cronet/ChromiumUrlFetcherTest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ public void testRequestComplete_withNon200StatusCode_callsCallbackWithException(
299299
verify(callback, timeout(1000)).onLoadFailed(captor.capture());
300300
assertThat(captor.getValue())
301301
.hasMessageThat()
302-
.isEqualTo("Http request failed with status code: 500");
302+
.isEqualTo("Http request failed, status code: 500");
303303
}
304304

305305
private void succeed(UrlResponseInfo info, Callback urlCallback, ByteBuffer byteBuffer)

‎library/src/main/java/com/bumptech/glide/load/HttpException.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,14 @@ public final class HttpException extends IOException {
1919
private final int statusCode;
2020

2121
public HttpException(int statusCode) {
22-
this("Http request failed with status code: " + statusCode, statusCode);
22+
this("Http request failed", statusCode);
2323
}
2424

25+
/**
26+
* @deprecated You should always include a status code, default to {@link #UNKNOWN} if you can't
27+
* come up with a reasonable one. This method will be removed in a future version.
28+
*/
29+
@Deprecated
2530
public HttpException(String message) {
2631
this(message, UNKNOWN);
2732
}
@@ -31,7 +36,7 @@ public HttpException(String message, int statusCode) {
3136
}
3237

3338
public HttpException(String message, int statusCode, @Nullable Throwable cause) {
34-
super(message, cause);
39+
super(message + ", status code: " + statusCode, cause);
3540
this.statusCode = statusCode;
3641
}
3742

‎library/src/main/java/com/bumptech/glide/load/data/HttpUrlFetcher.java

+77-32
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import java.io.IOException;
1515
import java.io.InputStream;
1616
import java.net.HttpURLConnection;
17+
import java.net.MalformedURLException;
1718
import java.net.URISyntaxException;
1819
import java.net.URL;
1920
import java.util.Map;
@@ -22,12 +23,13 @@
2223
public class HttpUrlFetcher implements DataFetcher<InputStream> {
2324
private static final String TAG = "HttpUrlFetcher";
2425
private static final int MAXIMUM_REDIRECTS = 5;
26+
@VisibleForTesting static final String REDIRECT_HEADER_FIELD = "Location";
2527

2628
@VisibleForTesting
2729
static final HttpUrlConnectionFactory DEFAULT_CONNECTION_FACTORY =
2830
new DefaultHttpUrlConnectionFactory();
2931
/** Returned when a connection error prevented us from receiving an http error. */
30-
private static final int INVALID_STATUS_CODE = -1;
32+
@VisibleForTesting static final int INVALID_STATUS_CODE = -1;
3133

3234
private final GlideUrl glideUrl;
3335
private final int timeout;
@@ -68,59 +70,97 @@ public void loadData(
6870
}
6971

7072
private InputStream loadDataWithRedirects(
71-
URL url, int redirects, URL lastUrl, Map<String, String> headers) throws IOException {
73+
URL url, int redirects, URL lastUrl, Map<String, String> headers) throws HttpException {
7274
if (redirects >= MAXIMUM_REDIRECTS) {
73-
throw new HttpException("Too many (> " + MAXIMUM_REDIRECTS + ") redirects!");
75+
throw new HttpException(
76+
"Too many (> " + MAXIMUM_REDIRECTS + ") redirects!", INVALID_STATUS_CODE);
7477
} else {
7578
// Comparing the URLs using .equals performs additional network I/O and is generally broken.
7679
// See http://michaelscharf.blogspot.com/2006/11/javaneturlequals-and-hashcode-make.html.
7780
try {
7881
if (lastUrl != null && url.toURI().equals(lastUrl.toURI())) {
79-
throw new HttpException("In re-direct loop");
82+
throw new HttpException("In re-direct loop", INVALID_STATUS_CODE);
8083
}
8184
} catch (URISyntaxException e) {
8285
// Do nothing, this is best effort.
8386
}
8487
}
8588

86-
urlConnection = connectionFactory.build(url);
87-
for (Map.Entry<String, String> headerEntry : headers.entrySet()) {
88-
urlConnection.addRequestProperty(headerEntry.getKey(), headerEntry.getValue());
89-
}
90-
urlConnection.setConnectTimeout(timeout);
91-
urlConnection.setReadTimeout(timeout);
92-
urlConnection.setUseCaches(false);
93-
urlConnection.setDoInput(true);
89+
urlConnection = buildAndConfigureConnection(url, headers);
9490

95-
// Stop the urlConnection instance of HttpUrlConnection from following redirects so that
96-
// redirects will be handled by recursive calls to this method, loadDataWithRedirects.
97-
urlConnection.setInstanceFollowRedirects(false);
91+
try {
92+
// Connect explicitly to avoid errors in decoders if connection fails.
93+
urlConnection.connect();
94+
// Set the stream so that it's closed in cleanup to avoid resource leaks. See #2352.
95+
stream = urlConnection.getInputStream();
96+
} catch (IOException e) {
97+
throw new HttpException(
98+
"Failed to connect or obtain data", getHttpStatusCodeOrInvalid(urlConnection), e);
99+
}
98100

99-
// Connect explicitly to avoid errors in decoders if connection fails.
100-
urlConnection.connect();
101-
// Set the stream so that it's closed in cleanup to avoid resource leaks. See #2352.
102-
stream = urlConnection.getInputStream();
103101
if (isCancelled) {
104102
return null;
105103
}
106-
final int statusCode = urlConnection.getResponseCode();
104+
105+
final int statusCode = getHttpStatusCodeOrInvalid(urlConnection);
107106
if (isHttpOk(statusCode)) {
108107
return getStreamForSuccessfulRequest(urlConnection);
109108
} else if (isHttpRedirect(statusCode)) {
110-
String redirectUrlString = urlConnection.getHeaderField("Location");
109+
String redirectUrlString = urlConnection.getHeaderField(REDIRECT_HEADER_FIELD);
111110
if (TextUtils.isEmpty(redirectUrlString)) {
112-
throw new HttpException("Received empty or null redirect url");
111+
throw new HttpException("Received empty or null redirect url", statusCode);
112+
}
113+
URL redirectUrl;
114+
try {
115+
redirectUrl = new URL(url, redirectUrlString);
116+
} catch (MalformedURLException e) {
117+
throw new HttpException("Bad redirect url: " + redirectUrlString, statusCode, e);
113118
}
114-
URL redirectUrl = new URL(url, redirectUrlString);
115119
// Closing the stream specifically is required to avoid leaking ResponseBodys in addition
116120
// to disconnecting the url connection below. See #2352.
117121
cleanup();
118122
return loadDataWithRedirects(redirectUrl, redirects + 1, url, headers);
119123
} else if (statusCode == INVALID_STATUS_CODE) {
120124
throw new HttpException(statusCode);
121125
} else {
122-
throw new HttpException(urlConnection.getResponseMessage(), statusCode);
126+
try {
127+
throw new HttpException(urlConnection.getResponseMessage(), statusCode);
128+
} catch (IOException e) {
129+
throw new HttpException("Failed to get a response message", statusCode, e);
130+
}
131+
}
132+
}
133+
134+
private static int getHttpStatusCodeOrInvalid(HttpURLConnection urlConnection) {
135+
try {
136+
return urlConnection.getResponseCode();
137+
} catch (IOException e) {
138+
if (Log.isLoggable(TAG, Log.DEBUG)) {
139+
Log.d(TAG, "Failed to get a response code", e);
140+
}
141+
}
142+
return INVALID_STATUS_CODE;
143+
}
144+
145+
private HttpURLConnection buildAndConfigureConnection(URL url, Map<String, String> headers)
146+
throws HttpException {
147+
HttpURLConnection urlConnection;
148+
try {
149+
urlConnection = connectionFactory.build(url);
150+
} catch (IOException e) {
151+
throw new HttpException("URL.openConnection threw", /*statusCode=*/ 0, e);
152+
}
153+
for (Map.Entry<String, String> headerEntry : headers.entrySet()) {
154+
urlConnection.addRequestProperty(headerEntry.getKey(), headerEntry.getValue());
123155
}
156+
urlConnection.setConnectTimeout(timeout);
157+
urlConnection.setReadTimeout(timeout);
158+
urlConnection.setUseCaches(false);
159+
urlConnection.setDoInput(true);
160+
// Stop the urlConnection instance of HttpUrlConnection from following redirects so that
161+
// redirects will be handled by recursive calls to this method, loadDataWithRedirects.
162+
urlConnection.setInstanceFollowRedirects(false);
163+
return urlConnection;
124164
}
125165

126166
// Referencing constants is less clear than a simple static method.
@@ -134,15 +174,20 @@ private static boolean isHttpRedirect(int statusCode) {
134174
}
135175

136176
private InputStream getStreamForSuccessfulRequest(HttpURLConnection urlConnection)
137-
throws IOException {
138-
if (TextUtils.isEmpty(urlConnection.getContentEncoding())) {
139-
int contentLength = urlConnection.getContentLength();
140-
stream = ContentLengthInputStream.obtain(urlConnection.getInputStream(), contentLength);
141-
} else {
142-
if (Log.isLoggable(TAG, Log.DEBUG)) {
143-
Log.d(TAG, "Got non empty content encoding: " + urlConnection.getContentEncoding());
177+
throws HttpException {
178+
try {
179+
if (TextUtils.isEmpty(urlConnection.getContentEncoding())) {
180+
int contentLength = urlConnection.getContentLength();
181+
stream = ContentLengthInputStream.obtain(urlConnection.getInputStream(), contentLength);
182+
} else {
183+
if (Log.isLoggable(TAG, Log.DEBUG)) {
184+
Log.d(TAG, "Got non empty content encoding: " + urlConnection.getContentEncoding());
185+
}
186+
stream = urlConnection.getInputStream();
144187
}
145-
stream = urlConnection.getInputStream();
188+
} catch (IOException e) {
189+
throw new HttpException(
190+
"Failed to obtain InputStream", getHttpStatusCodeOrInvalid(urlConnection), e);
146191
}
147192
return stream;
148193
}

‎library/test/src/test/java/com/bumptech/glide/load/data/HttpUrlFetcherTest.java

+70-4
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,28 @@
11
package com.bumptech.glide.load.data;
22

3+
import static com.google.common.truth.Truth.assertThat;
34
import static org.mockito.ArgumentMatchers.eq;
45
import static org.mockito.ArgumentMatchers.isNull;
6+
import static org.mockito.Mockito.doThrow;
57
import static org.mockito.Mockito.inOrder;
68
import static org.mockito.Mockito.never;
79
import static org.mockito.Mockito.verify;
810
import static org.mockito.Mockito.when;
911

1012
import com.bumptech.glide.Priority;
13+
import com.bumptech.glide.load.HttpException;
1114
import com.bumptech.glide.load.model.GlideUrl;
1215
import java.io.ByteArrayInputStream;
16+
import java.io.FileNotFoundException;
1317
import java.io.IOException;
1418
import java.io.InputStream;
1519
import java.net.HttpURLConnection;
20+
import java.net.MalformedURLException;
1621
import java.net.URL;
1722
import org.junit.Before;
1823
import org.junit.Test;
1924
import org.junit.runner.RunWith;
25+
import org.mockito.ArgumentCaptor;
2026
import org.mockito.InOrder;
2127
import org.mockito.Mock;
2228
import org.mockito.MockitoAnnotations;
@@ -49,13 +55,73 @@ public void setUp() throws IOException {
4955
}
5056

5157
@Test
52-
public void testSetsReadTimeout() throws IOException {
58+
public void loadData_whenConnectThrowsFileNotFound_notifiesCallbackWithHttpErrorCode()
59+
throws IOException {
60+
int statusCode = 400;
61+
doThrow(new FileNotFoundException()).when(urlConnection).connect();
62+
when(urlConnection.getResponseCode()).thenReturn(statusCode);
63+
64+
fetcher.loadData(Priority.HIGH, callback);
65+
66+
HttpException exception = (HttpException) getCallbackException();
67+
assertThat(exception.getStatusCode()).isEqualTo(statusCode);
68+
}
69+
70+
@Test
71+
public void loadData_whenGetInputStreamThrows_notifiesCallbackWithStatusCode()
72+
throws IOException {
73+
int statusCode = 400;
74+
doThrow(new IOException()).when(urlConnection).getInputStream();
75+
when(urlConnection.getResponseCode()).thenReturn(statusCode);
76+
77+
fetcher.loadData(Priority.HIGH, callback);
78+
79+
HttpException exception = (HttpException) getCallbackException();
80+
assertThat(exception.getStatusCode()).isEqualTo(statusCode);
81+
}
82+
83+
@Test
84+
public void loadData_whenConnectAndGetResponseCodeThrow_notifiesCallbackWithInvalidStatusCode()
85+
throws IOException {
86+
doThrow(new FileNotFoundException()).when(urlConnection).connect();
87+
when(urlConnection.getResponseCode()).thenThrow(new IOException());
88+
89+
fetcher.loadData(Priority.HIGH, callback);
90+
91+
HttpException exception = (HttpException) getCallbackException();
92+
assertThat(exception.getStatusCode()).isEqualTo(HttpUrlFetcher.INVALID_STATUS_CODE);
93+
}
94+
95+
@Test
96+
public void loadData_whenRedirectUrlIsMalformed_notifiesCallbackWithStatusCode()
97+
throws IOException {
98+
int statusCode = 300;
99+
100+
when(urlConnection.getHeaderField(eq(HttpUrlFetcher.REDIRECT_HEADER_FIELD)))
101+
.thenReturn("gg://www.google.com");
102+
when(urlConnection.getResponseCode()).thenReturn(statusCode);
103+
104+
fetcher.loadData(Priority.HIGH, callback);
105+
106+
HttpException exception = (HttpException) getCallbackException();
107+
assertThat(exception.getStatusCode()).isEqualTo(statusCode);
108+
assertThat(exception.getCause()).isInstanceOf(MalformedURLException.class);
109+
}
110+
111+
private Exception getCallbackException() {
112+
ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class);
113+
verify(callback).onLoadFailed(captor.capture());
114+
return captor.getValue();
115+
}
116+
117+
@Test
118+
public void testSetsReadTimeout() {
53119
fetcher.loadData(Priority.HIGH, callback);
54120
verify(urlConnection).setReadTimeout(eq(TIMEOUT_MS));
55121
}
56122

57123
@Test
58-
public void testSetsConnectTimeout() throws IOException {
124+
public void testSetsConnectTimeout() {
59125
fetcher.loadData(Priority.IMMEDIATE, callback);
60126
verify(urlConnection).setConnectTimeout(eq(TIMEOUT_MS));
61127
}
@@ -71,7 +137,7 @@ public void testReturnsNullIfCancelledBeforeConnects() throws IOException {
71137
}
72138

73139
@Test
74-
public void testDisconnectsUrlOnCleanup() throws IOException {
140+
public void testDisconnectsUrlOnCleanup() {
75141
fetcher.loadData(Priority.HIGH, callback);
76142
fetcher.cleanup();
77143

@@ -89,7 +155,7 @@ public void testDoesNotThrowIfCancelCalledBeforeStart() {
89155
}
90156

91157
@Test
92-
public void testCancelDoesNotDisconnectIfAlreadyConnected() throws IOException {
158+
public void testCancelDoesNotDisconnectIfAlreadyConnected() {
93159
fetcher.loadData(Priority.HIGH, callback);
94160
fetcher.cancel();
95161

0 commit comments

Comments
 (0)
Please sign in to comment.