Skip to content

Commit a9bb930

Browse files
bitwisemanbitwiseman
committed
Move cached 404 retry to main code path
1 parent 30c70bc commit a9bb930

1 file changed

Lines changed: 52 additions & 47 deletions

File tree

‎src/main/java/org/kohsuke/github/Requester.java‎

Lines changed: 52 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
import static java.util.logging.Level.*;
6767
import static org.apache.commons.lang3.StringUtils.defaultString;
6868
import static org.kohsuke.github.GitHub.MAPPER;
69+
import static org.kohsuke.github.GitHub.connect;
6970

7071
/**
7172
* A builder pattern for making HTTP call and parsing its output.
@@ -487,9 +488,10 @@ private <T> T _fetch(SupplierThrows<T, IOException> supplier) throws IOException
487488

488489
private <T> T _fetch(String tailApiUrl, URL url, SupplierThrows<T, IOException> supplier) throws IOException {
489490
while (true) {// loop while API rate limit is hit
490-
setupConnection(url);
491+
uc = setupConnection(url);
491492

492493
try {
494+
retryInvalidCached404Response();
493495
return supplier.get();
494496
} catch (IOException e) {
495497
handleApiError(e);
@@ -608,24 +610,25 @@ public String getResponseHeader(String header) {
608610
/**
609611
* Set up the request parameters or POST payload.
610612
*/
611-
private void buildRequest() throws IOException {
613+
private void buildRequest(HttpURLConnection connection) throws IOException {
612614
if (isMethodWithBody()) {
613-
uc.setDoOutput(true);
615+
connection.setDoOutput(true);
614616

615617
if (body == null) {
616-
uc.setRequestProperty("Content-type", defaultString(contentType, "application/json"));
618+
connection.setRequestProperty("Content-type", defaultString(contentType, "application/json"));
617619
Map json = new HashMap();
618620
for (Entry e : args) {
619621
json.put(e.key, e.value);
620622
}
621-
MAPPER.writeValue(uc.getOutputStream(), json);
623+
MAPPER.writeValue(connection.getOutputStream(), json);
622624
} else {
623-
uc.setRequestProperty("Content-type", defaultString(contentType, "application/x-www-form-urlencoded"));
625+
connection.setRequestProperty("Content-type",
626+
defaultString(contentType, "application/x-www-form-urlencoded"));
624627
try {
625628
byte[] bytes = new byte[32768];
626629
int read;
627630
while ((read = body.read(bytes)) != -1) {
628-
uc.getOutputStream().write(bytes, 0, read);
631+
connection.getOutputStream().write(bytes, 0, read);
629632
}
630633
} finally {
631634
body.close();
@@ -782,47 +785,49 @@ private void findNextURL() throws MalformedURLException {
782785
}
783786
}
784787

785-
private void setupConnection(URL url) throws IOException {
788+
private HttpURLConnection setupConnection(URL url) throws IOException {
786789
if (LOGGER.isLoggable(FINE)) {
787790
LOGGER.log(FINE,
788791
"GitHub API request [" + (root.login == null ? "anonymous" : root.login) + "]: " + method + " "
789792
+ url.toString());
790793
}
791-
uc = root.getConnector().connect(url);
794+
HttpURLConnection connection = root.getConnector().connect(url);
792795

793796
// if the authentication is needed but no credential is given, try it anyway (so that some calls
794797
// that do work with anonymous access in the reduced form should still work.)
795798
if (root.encodedAuthorization != null)
796-
uc.setRequestProperty("Authorization", root.encodedAuthorization);
799+
connection.setRequestProperty("Authorization", root.encodedAuthorization);
797800

798801
for (Map.Entry<String, String> e : headers.entrySet()) {
799802
String v = e.getValue();
800803
if (v != null)
801-
uc.setRequestProperty(e.getKey(), v);
804+
connection.setRequestProperty(e.getKey(), v);
802805
}
803806

804-
setRequestMethod(uc);
805-
uc.setRequestProperty("Accept-Encoding", "gzip");
806-
buildRequest();
807+
setRequestMethod(connection);
808+
connection.setRequestProperty("Accept-Encoding", "gzip");
809+
buildRequest(connection);
810+
811+
return connection;
807812
}
808813

809-
private void setRequestMethod(HttpURLConnection uc) throws IOException {
814+
private void setRequestMethod(HttpURLConnection connection) throws IOException {
810815
try {
811-
uc.setRequestMethod(method);
816+
connection.setRequestMethod(method);
812817
} catch (ProtocolException e) {
813818
// JDK only allows one of the fixed set of verbs. Try to override that
814819
try {
815820
Field $method = HttpURLConnection.class.getDeclaredField("method");
816821
$method.setAccessible(true);
817-
$method.set(uc, method);
822+
$method.set(connection, method);
818823
} catch (Exception x) {
819824
throw (IOException) new IOException("Failed to set the custom verb").initCause(x);
820825
}
821826
// sun.net.www.protocol.https.DelegatingHttpsURLConnection delegates to another HttpURLConnection
822827
try {
823-
Field $delegate = uc.getClass().getDeclaredField("delegate");
828+
Field $delegate = connection.getClass().getDeclaredField("delegate");
824829
$delegate.setAccessible(true);
825-
Object delegate = $delegate.get(uc);
830+
Object delegate = $delegate.get(connection);
826831
if (delegate instanceof HttpURLConnection) {
827832
HttpURLConnection nested = (HttpURLConnection) delegate;
828833
setRequestMethod(nested);
@@ -833,7 +838,7 @@ private void setRequestMethod(HttpURLConnection uc) throws IOException {
833838
throw (IOException) new IOException("Failed to set the custom verb").initCause(x);
834839
}
835840
}
836-
if (!uc.getRequestMethod().equals(method))
841+
if (!connection.getRequestMethod().equals(method))
837842
throw new IllegalStateException("Failed to set the request method to " + method);
838843
}
839844

@@ -885,39 +890,16 @@ private <T> T parse(Class<T> type, T instance, int timeouts) throws IOException
885890
try {
886891
return setResponseHeaders(MAPPER.readValue(data, type));
887892
} catch (JsonMappingException e) {
888-
throw (IOException) new IOException("Failed to deserialize " + data).initCause(e);
893+
String message = "Failed to deserialize " + data;
894+
throw (IOException) new IOException(message).initCause(e);
889895
}
890896
if (instance != null) {
891897
return setResponseHeaders(MAPPER.readerForUpdating(instance).<T>readValue(data));
892898
}
893899
return null;
894900
} catch (FileNotFoundException e) {
895-
// java.net.URLConnection handles 404 exception has FileNotFoundException, don't wrap exception in
896-
// HttpException
897-
// to preserve backward compatibility
898-
899-
// WORKAROUND FOR ISSUE #669:
900-
// When the Requester detects a 404 response with an ETag (only happpens when the server's 304
901-
// is bogus and would cause cache corruption), try the query again with new request header
902-
// that forces the server to not return 304 and return new data instead.
903-
//
904-
// This solution is transparent to users of this library and automatically handles a
905-
// situation that was cause insidious and hard to debug bad responses in caching
906-
// scenarios. If GitHub ever fixes their issue and/or begins providing accurate ETags to
907-
// their 404 responses, this will result in at worst two requests being made for each 404
908-
// responses. However, only the second request will count against rate limit.
909-
910-
// If we tried this once already, don't try again.
911-
if (Objects.equals(uc.getRequestMethod(), "GET") && uc.getHeaderField("ETag") != null
912-
&& !Objects.equals(uc.getRequestProperty("Cache-Control"), "no-cache") && timeouts > 0) {
913-
setupConnection(uc.getURL());
914-
// Setting "Cache-Control" to "no-cache" stops the cache from supplying
915-
// "If-Modified-Since" or "If-None-Match" values.
916-
// This makes GitHub give us current data (not incorrectly cached data)
917-
uc.setRequestProperty("Cache-Control", "no-cache");
918-
return parse(type, instance, timeouts - 1);
919-
}
920-
901+
// java.net.URLConnection handles 404 exception as FileNotFoundException,
902+
// don't wrap exception in HttpException to preserve backward compatibility
921903
throw e;
922904
} catch (IOException e) {
923905
if (e instanceof SocketTimeoutException && timeouts > 0) {
@@ -930,6 +912,29 @@ private <T> T parse(Class<T> type, T instance, int timeouts) throws IOException
930912
}
931913
}
932914

915+
private void retryInvalidCached404Response() throws IOException {
916+
// WORKAROUND FOR ISSUE #669:
917+
// When the Requester detects a 404 response with an ETag (only happpens when the server's 304
918+
// is bogus and would cause cache corruption), try the query again with new request header
919+
// that forces the server to not return 304 and return new data instead.
920+
//
921+
// This solution is transparent to users of this library and automatically handles a
922+
// situation that was cause insidious and hard to debug bad responses in caching
923+
// scenarios. If GitHub ever fixes their issue and/or begins providing accurate ETags to
924+
// their 404 responses, this will result in at worst two requests being made for each 404
925+
// responses. However, only the second request will count against rate limit.
926+
int responseCode = uc.getResponseCode();
927+
if (responseCode == 404 && Objects.equals(uc.getRequestMethod(), "GET") && uc.getHeaderField("ETag") != null
928+
&& !Objects.equals(uc.getRequestProperty("Cache-Control"), "no-cache")) {
929+
uc = setupConnection(uc.getURL());
930+
// Setting "Cache-Control" to "no-cache" stops the cache from supplying
931+
// "If-Modified-Since" or "If-None-Match" values.
932+
// This makes GitHub give us current data (not incorrectly cached data)
933+
uc.setRequestProperty("Cache-Control", "no-cache");
934+
uc.getResponseCode();
935+
}
936+
}
937+
933938
private <T> T setResponseHeaders(T readValue) {
934939
if (readValue instanceof GHObject[]) {
935940
for (GHObject ghObject : (GHObject[]) readValue) {

0 commit comments

Comments
 (0)