Skip to content

Commit ec31e94

Browse files
bitwisemanbitwiseman
authored
Merge pull request hub4j#665 from bitwiseman/task/cache-error-test
Workaround for `If-Modified-Since` HTTP request header causing cache corruption
2 parents ea631d0 + 66a1803 commit ec31e94

28 files changed

Lines changed: 1665 additions & 0 deletions

File tree

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -895,6 +895,29 @@ private <T> T parse(Class<T> type, T instance, int timeouts) throws IOException
895895
// java.net.URLConnection handles 404 exception has FileNotFoundException, don't wrap exception in
896896
// HttpException
897897
// 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+
898921
throw e;
899922
} catch (IOException e) {
900923
if (e instanceof SocketTimeoutException && timeouts > 0) {

‎src/test/java/org/kohsuke/github/extras/GitHubCachingTest.java‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
package org.kohsuke.github.extras;
2+
3+
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
4+
import com.github.tomakehurst.wiremock.extension.responsetemplating.ResponseTemplateTransformer;
5+
import com.squareup.okhttp.Cache;
6+
import com.squareup.okhttp.OkHttpClient;
7+
import com.squareup.okhttp.OkUrlFactory;
8+
import org.apache.commons.io.FileUtils;
9+
import org.junit.Before;
10+
import org.junit.Test;
11+
import org.kohsuke.github.AbstractGitHubWireMockTest;
12+
import org.kohsuke.github.GHFileNotFoundException;
13+
import org.kohsuke.github.GHIssueState;
14+
import org.kohsuke.github.GHPullRequest;
15+
import org.kohsuke.github.GHRef;
16+
import org.kohsuke.github.GHRepository;
17+
import org.kohsuke.github.GitHub;
18+
19+
import java.io.File;
20+
import java.io.IOException;
21+
22+
/**
23+
* Test showing the behavior of OkHttpConnector cache with GitHub 404 responses.
24+
*
25+
* @author Liam Newman
26+
*/
27+
public class GitHubCachingTest extends AbstractGitHubWireMockTest {
28+
29+
public GitHubCachingTest() {
30+
useDefaultGitHub = false;
31+
}
32+
33+
String testRefName = "heads/test/content_ref_cache";
34+
35+
@Override
36+
protected WireMockConfiguration getWireMockOptions() {
37+
return super.getWireMockOptions()
38+
.extensions(ResponseTemplateTransformer.builder().global(true).maxCacheEntries(0L).build());
39+
}
40+
41+
@Before
42+
public void setupRepo() throws Exception {
43+
if (mockGitHub.isUseProxy()) {
44+
for (GHPullRequest pr : getRepository(this.gitHubBeforeAfter).getPullRequests(GHIssueState.OPEN)) {
45+
pr.close();
46+
}
47+
try {
48+
GHRef ref = getRepository(this.gitHubBeforeAfter).getRef(testRefName);
49+
ref.delete();
50+
} catch (IOException e) {
51+
}
52+
}
53+
}
54+
55+
@Test
56+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubRef_Error() throws Exception {
57+
// ISSUE #669
58+
snapshotNotAllowed();
59+
60+
OkHttpClient client = createClient(true);
61+
OkHttpConnector connector = new OkHttpConnector(new OkUrlFactory(client));
62+
63+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
64+
.withConnector(connector)
65+
.build();
66+
67+
// Alternate client also doing caching but staying in a good state
68+
// We use this to do sanity checks and other information gathering
69+
GitHub gitHub2 = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
70+
.withConnector(new OkHttpConnector(new OkUrlFactory(createClient(true))))
71+
.build();
72+
73+
// Create a branch from a known conflicting branch
74+
GHRepository repo = getRepository(gitHub);
75+
76+
String baseSha = repo.getRef("heads/test/unmergeable").getObject().getSha();
77+
78+
GHRef ref;
79+
ref = repo.createRef("refs/" + testRefName, baseSha);
80+
81+
// Verify we can query the created ref
82+
ref = repo.getRef(testRefName);
83+
84+
// Verify we can query the created ref from cache
85+
ref = repo.getRef(testRefName);
86+
87+
// Delete the ref
88+
ref.delete();
89+
90+
// This is just to show this isn't a race condition
91+
Thread.sleep(2000);
92+
93+
// Try to get the non-existant ref (GHFileNotFound)
94+
try {
95+
repo.getRef(testRefName);
96+
fail();
97+
} catch (GHFileNotFoundException e) {
98+
// expected
99+
100+
// FYI: Querying again when the item is actually not present does not produce a 304
101+
// It produces another 404,
102+
// Try to get the non-existant ref (GHFileNotFound)
103+
try {
104+
repo.getRef(testRefName);
105+
fail();
106+
} catch (GHFileNotFoundException ex) {
107+
// expected
108+
}
109+
110+
}
111+
112+
// This is just to show this isn't a race condition
113+
Thread.sleep(2000);
114+
115+
ref = repo.createRef("refs/" + testRefName, baseSha);
116+
117+
// Verify ref exists and can be queried from uncached connection
118+
// Expected: success
119+
// Actual: still GHFileNotFound due to caching: GitHub incorrectly returns 304
120+
// even though contents of the ref have changed.
121+
//
122+
// There source of this issue seems to be that 404's do not return an ETAG,
123+
// so the cache falls back to using "If-Modified-Since" which is erroneously returns a 304.
124+
//
125+
// NOTE: This is even worse than you might think: 404 responses don't return an ETAG, but 304 responses do.
126+
//
127+
// Due erroneous 304 returned from "If-Modified-Since", the ETAG returned by the first 304
128+
// is actually the ETAG for the NEW state of the ref query (the one where the ref exists).
129+
// This can be verified by comparing the ETAG from gitHub2 client to the ETAG in error.
130+
//
131+
// This means that server thinks it telling the client that the new state is stable
132+
// while the cache thinks it confirming the old state hasn't changed.
133+
//
134+
// So, after the first 304, the failure is locked in via ETAG and won't until the ref is modified again
135+
// or until the cache ages out entry without the URL being requeried (which is why users report that refreshing
136+
// is now help).
137+
138+
try {
139+
repo.getRef(testRefName);
140+
} catch (GHFileNotFoundException e) {
141+
// Sanity check: ref exists and can be queried from other client
142+
getRepository(gitHub2).getRef(testRefName);
143+
144+
// We're going to fail, query again to see the incorrect ETAG cached from first query being used
145+
// It is the same ETAG as the one returned to the second client.
146+
// Now we're in trouble.
147+
repo.getRef(testRefName);
148+
149+
// We should never fail the first query and pass the second,
150+
// the test has still failed if it get here.
151+
fail();
152+
}
153+
154+
// OMG, the workaround succeeded!
155+
// This correct response should be generated from a 304.
156+
repo.getRef(testRefName);
157+
}
158+
159+
private static int clientCount = 0;
160+
161+
private OkHttpClient createClient(boolean useCache) throws IOException {
162+
OkHttpClient client = new OkHttpClient();
163+
164+
if (useCache) {
165+
File cacheDir = new File(
166+
"target/cache/" + baseFilesClassPath + "/" + mockGitHub.getMethodName() + clientCount++);
167+
cacheDir.mkdirs();
168+
FileUtils.cleanDirectory(cacheDir);
169+
Cache cache = new Cache(cacheDir, 100 * 1024L * 1024L);
170+
171+
client.setCache(cache);
172+
}
173+
174+
return client;
175+
}
176+
177+
private static GHRepository getRepository(GitHub gitHub) throws IOException {
178+
return gitHub.getOrganization("github-api-test-org").getRepository("github-api");
179+
}
180+
181+
}

‎src/test/java/org/kohsuke/github/extras/okhttp3/GitHubCachingTest.java‎

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
package org.kohsuke.github.extras.okhttp3;
2+
3+
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
4+
import com.github.tomakehurst.wiremock.extension.responsetemplating.ResponseTemplateTransformer;
5+
import okhttp3.Cache;
6+
import okhttp3.OkHttpClient;
7+
import org.apache.commons.io.FileUtils;
8+
import org.junit.Before;
9+
import org.junit.Test;
10+
import org.kohsuke.github.AbstractGitHubWireMockTest;
11+
import org.kohsuke.github.GHFileNotFoundException;
12+
import org.kohsuke.github.GHIssueState;
13+
import org.kohsuke.github.GHPullRequest;
14+
import org.kohsuke.github.GHRef;
15+
import org.kohsuke.github.GHRepository;
16+
import org.kohsuke.github.GitHub;
17+
18+
import java.io.File;
19+
import java.io.IOException;
20+
21+
/**
22+
* Test showing the behavior of OkHttpConnector cache with GitHub 404 responses.
23+
*
24+
* @author Liam Newman
25+
*/
26+
public class GitHubCachingTest extends AbstractGitHubWireMockTest {
27+
28+
public GitHubCachingTest() {
29+
useDefaultGitHub = false;
30+
}
31+
32+
String testRefName = "heads/test/content_ref_cache";
33+
34+
@Override
35+
protected WireMockConfiguration getWireMockOptions() {
36+
return super.getWireMockOptions()
37+
// Use the same data files as the 2.x test
38+
.usingFilesUnderDirectory(baseRecordPath.replace("/okhttp3/", "/"))
39+
.extensions(ResponseTemplateTransformer.builder().global(true).maxCacheEntries(0L).build());
40+
}
41+
42+
@Before
43+
public void setupRepo() throws Exception {
44+
if (mockGitHub.isUseProxy()) {
45+
for (GHPullRequest pr : getRepository(this.gitHubBeforeAfter).getPullRequests(GHIssueState.OPEN)) {
46+
pr.close();
47+
}
48+
try {
49+
GHRef ref = getRepository(this.gitHubBeforeAfter).getRef(testRefName);
50+
ref.delete();
51+
} catch (IOException e) {
52+
}
53+
}
54+
}
55+
56+
@Test
57+
public void OkHttpConnector_Cache_MaxAgeDefault_Zero_GitHubRef_Error() throws Exception {
58+
// ISSUE #669
59+
snapshotNotAllowed();
60+
61+
OkHttpClient client = createClient(true);
62+
OkHttpConnector connector = new OkHttpConnector(client);
63+
64+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
65+
.withConnector(connector)
66+
.build();
67+
68+
// Alternate client also doing caching but staying in a good state
69+
// We use this to do sanity checks and other information gathering
70+
GitHub gitHub2 = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
71+
.withConnector(new OkHttpConnector(createClient(true)))
72+
.build();
73+
74+
// Create a branch from a known conflicting branch
75+
GHRepository repo = getRepository(gitHub);
76+
77+
String baseSha = repo.getRef("heads/test/unmergeable").getObject().getSha();
78+
79+
GHRef ref;
80+
ref = repo.createRef("refs/" + testRefName, baseSha);
81+
82+
// Verify we can query the created ref
83+
ref = repo.getRef(testRefName);
84+
85+
// Verify we can query the created ref from cache
86+
ref = repo.getRef(testRefName);
87+
88+
// Delete the ref
89+
ref.delete();
90+
91+
// This is just to show this isn't a race condition
92+
Thread.sleep(2000);
93+
94+
// Try to get the non-existant ref (GHFileNotFound)
95+
try {
96+
repo.getRef(testRefName);
97+
fail();
98+
} catch (GHFileNotFoundException e) {
99+
// expected
100+
101+
// FYI: Querying again when the item is actually not present does not produce a 304
102+
// It produces another 404,
103+
// Try to get the non-existant ref (GHFileNotFound)
104+
try {
105+
repo.getRef(testRefName);
106+
fail();
107+
} catch (GHFileNotFoundException ex) {
108+
// expected
109+
}
110+
111+
}
112+
113+
// This is just to show this isn't a race condition
114+
Thread.sleep(2000);
115+
116+
ref = repo.createRef("refs/" + testRefName, baseSha);
117+
118+
// Verify ref exists and can be queried from uncached connection
119+
// Expected: success
120+
// Actual: still GHFileNotFound due to caching: GitHub incorrectly returns 304
121+
// even though contents of the ref have changed.
122+
//
123+
// There source of this issue seems to be that 404's do not return an ETAG,
124+
// so the cache falls back to using "If-Modified-Since" which is erroneously returns a 304.
125+
//
126+
// NOTE: This is even worse than you might think: 404 responses don't return an ETAG, but 304 responses do.
127+
//
128+
// Due erroneous 304 returned from "If-Modified-Since", the ETAG returned by the first 304
129+
// is actually the ETAG for the NEW state of the ref query (the one where the ref exists).
130+
// This can be verified by comparing the ETAG from gitHub2 client to the ETAG in error.
131+
//
132+
// This means that server thinks it telling the client that the new state is stable
133+
// while the cache thinks it confirming the old state hasn't changed.
134+
//
135+
// So, after the first 304, the failure is locked in via ETAG and won't until the ref is modified again
136+
// or until the cache ages out entry without the URL being requeried (which is why users report that refreshing
137+
// is now help).
138+
139+
try {
140+
repo.getRef(testRefName);
141+
} catch (GHFileNotFoundException e) {
142+
// Sanity check: ref exists and can be queried from other client
143+
getRepository(gitHub2).getRef(testRefName);
144+
145+
// We're going to fail, query again to see the incorrect ETAG cached from first query being used
146+
// It is the same ETAG as the one returned to the second client.
147+
// Now we're in trouble.
148+
repo.getRef(testRefName);
149+
150+
// We should never fail the first query and pass the second,
151+
// the test has still failed if it get here.
152+
fail();
153+
}
154+
155+
// OMG, the workaround succeeded!
156+
// This correct response should be generated from a 304.
157+
repo.getRef(testRefName);
158+
}
159+
160+
private static int clientCount = 0;
161+
162+
private OkHttpClient createClient(boolean useCache) throws IOException {
163+
OkHttpClient.Builder builder = new OkHttpClient().newBuilder();
164+
165+
if (useCache) {
166+
File cacheDir = new File(
167+
"target/cache/" + baseFilesClassPath + "/" + mockGitHub.getMethodName() + clientCount++);
168+
cacheDir.mkdirs();
169+
FileUtils.cleanDirectory(cacheDir);
170+
Cache cache = new Cache(cacheDir, 100 * 1024L * 1024L);
171+
172+
builder.cache(cache);
173+
}
174+
175+
return builder.build();
176+
}
177+
178+
private static GHRepository getRepository(GitHub gitHub) throws IOException {
179+
return gitHub.getOrganization("github-api-test-org").getRepository("github-api");
180+
}
181+
182+
}

0 commit comments

Comments
 (0)