Skip to content

Commit eb4000f

Browse files
bitwisemanbitwiseman
committed
Streamline retry code path
1 parent 74dd887 commit eb4000f

1 file changed

Lines changed: 42 additions & 23 deletions

File tree

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

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@
7676
* @author Kohsuke Kawaguchi
7777
*/
7878
class Requester {
79-
public static final int SOCKET_ERROR_RETRIES = 2;
79+
public static final int CONNECTION_ERROR_RETRIES = 2;
8080
private final GitHub root;
8181
private final List<Entry> args = new ArrayList<Entry>();
8282
private final Map<String, String> headers = new LinkedHashMap<String, String>();
@@ -499,7 +499,7 @@ private <T> T _fetch(String tailApiUrl, URL url, SupplierThrows<T, IOException>
499499
uc = setupConnection(url);
500500

501501
try {
502-
return _fetchOrRetry(supplier, SOCKET_ERROR_RETRIES);
502+
return _fetchOrRetry(supplier, CONNECTION_ERROR_RETRIES);
503503
} catch (IOException e) {
504504
handleApiError(e);
505505
} finally {
@@ -511,35 +511,50 @@ private <T> T _fetch(String tailApiUrl, URL url, SupplierThrows<T, IOException>
511511
private <T> T _fetchOrRetry(SupplierThrows<T, IOException> supplier, int retries) throws IOException {
512512
int responseCode = -1;
513513
String responseMessage = null;
514+
// When retries equal 0 the previous call must return or throw, not retry again
515+
if (retries < 0) {
516+
throw new IllegalArgumentException("'retries' cannot be less than 0");
517+
}
518+
514519
try {
515520
// This is where the request is sent and response is processing starts
516521
responseCode = uc.getResponseCode();
517-
518-
// If we are caching and get an invalid cached 404, retry it.
519-
responseCode = retryIfInvalidCached404Response(responseCode);
520522
responseMessage = uc.getResponseMessage();
521523

522-
return supplier.get();
524+
// If we are caching and get an invalid cached 404, retry it.
525+
if (!retryInvalidCached404Response(responseCode, retries)) {
526+
return supplier.get();
527+
}
523528
} catch (FileNotFoundException e) {
524529
// java.net.URLConnection handles 404 exception as FileNotFoundException,
525530
// don't wrap exception in HttpException to preserve backward compatibility
526531
throw e;
527532
} catch (IOException e) {
528-
if ((e instanceof SocketException || e instanceof SocketTimeoutException) && retries > 0) {
529-
LOGGER.log(INFO,
530-
"timed out accessing " + uc.getURL() + ". Sleeping " + Requester.retryTimeoutMillis
531-
+ " milliseconds before retrying... ; will try " + retries + " more time(s)",
532-
e);
533-
try {
534-
Thread.sleep(Requester.retryTimeoutMillis);
535-
} catch (InterruptedException ie) {
536-
throw (IOException) new InterruptedIOException().initCause(e);
537-
}
538-
uc = setupConnection(uc.getURL());
539-
return _fetchOrRetry(supplier, retries - 1);
533+
if (!retrySocketException(e, retries)) {
534+
throw new HttpException(responseCode, responseMessage, uc.getURL(), e);
535+
}
536+
}
537+
538+
// We did not fetch or throw, retry
539+
return _fetchOrRetry(supplier, retries - 1);
540+
541+
}
542+
543+
private boolean retrySocketException(IOException e, int retries) throws IOException {
544+
if ((e instanceof SocketException || e instanceof SocketTimeoutException) && retries > 0) {
545+
LOGGER.log(INFO,
546+
"timed out accessing " + uc.getURL() + ". Sleeping " + Requester.retryTimeoutMillis
547+
+ " milliseconds before retrying... ; will try " + retries + " more time(s)",
548+
e);
549+
try {
550+
Thread.sleep(Requester.retryTimeoutMillis);
551+
} catch (InterruptedException ie) {
552+
throw (IOException) new InterruptedIOException().initCause(e);
540553
}
541-
throw new HttpException(responseCode, responseMessage, uc.getURL(), e);
554+
uc = setupConnection(uc.getURL());
555+
return true;
542556
}
557+
return false;
543558
}
544559

545560
private <T> T[] concatenatePages(Class<T[]> type, List<T[]> pages, int totalLength) {
@@ -941,7 +956,7 @@ private <T> T parse(Class<T> type, T instance, int timeouts) throws IOException
941956
}
942957
}
943958

944-
private int retryIfInvalidCached404Response(int responseCode) throws IOException {
959+
private boolean retryInvalidCached404Response(int responseCode, int retries) throws IOException {
945960
// WORKAROUND FOR ISSUE #669:
946961
// When the Requester detects a 404 response with an ETag (only happpens when the server's 304
947962
// is bogus and would cause cache corruption), try the query again with new request header
@@ -953,15 +968,19 @@ private int retryIfInvalidCached404Response(int responseCode) throws IOException
953968
// their 404 responses, this will result in at worst two requests being made for each 404
954969
// responses. However, only the second request will count against rate limit.
955970
if (responseCode == 404 && Objects.equals(uc.getRequestMethod(), "GET") && uc.getHeaderField("ETag") != null
956-
&& !Objects.equals(uc.getRequestProperty("Cache-Control"), "no-cache")) {
971+
&& !Objects.equals(uc.getRequestProperty("Cache-Control"), "no-cache") && retries > 0) {
972+
LOGGER.log(FINE,
973+
"Encountered GitHub invalid cached 404 from " + uc.getURL()
974+
+ ". Retrying with \"Cache-Control\"=\"no-cache\"...");
975+
957976
uc = setupConnection(uc.getURL());
958977
// Setting "Cache-Control" to "no-cache" stops the cache from supplying
959978
// "If-Modified-Since" or "If-None-Match" values.
960979
// This makes GitHub give us current data (not incorrectly cached data)
961980
uc.setRequestProperty("Cache-Control", "no-cache");
962-
responseCode = uc.getResponseCode();
981+
return true;
963982
}
964-
return responseCode;
983+
return false;
965984
}
966985

967986
private <T> T setResponseHeaders(T readValue) {

0 commit comments

Comments
 (0)