Skip to content

Commit 3059575

Browse files
bitwisemanbitwiseman
authored
Merge pull request #271 from bitwiseman/issue/JENKINS-54126
Remove local workaround for JENKINS-54126, use github-api fix
2 parents 9417993 + bb54ab8 commit 3059575

File tree

12 files changed

+509
-138
lines changed

12 files changed

+509
-138
lines changed

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@
162162
<dependency>
163163
<groupId>com.github.tomakehurst</groupId>
164164
<artifactId>wiremock-jre8-standalone</artifactId>
165-
<version>2.24.1</version>
165+
<version>2.25.1</version>
166166
<scope>test</scope>
167167
</dependency>
168168
<dependency>

‎src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java‎

Lines changed: 1 addition & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
3737
import com.cloudbees.plugins.credentials.domains.URIRequirementBuilder;
3838
import com.squareup.okhttp.Cache;
39-
import com.squareup.okhttp.CacheControl;
4039
import com.squareup.okhttp.OkHttpClient;
4140
import com.squareup.okhttp.OkUrlFactory;
4241
import edu.umd.cs.findbugs.annotations.NonNull;
@@ -47,7 +46,6 @@
4746
import hudson.model.PeriodicWork;
4847
import hudson.model.Queue;
4948
import hudson.model.TaskListener;
50-
import hudson.model.queue.Tasks;
5149
import hudson.security.ACL;
5250
import hudson.util.FormValidation;
5351
import hudson.util.ListBoxModel;
@@ -394,12 +392,7 @@ public static void checkApiUrlValidity(@Nonnull GitHub gitHub, @CheckForNull Sta
394392
}
395393
}
396394

397-
if (client.getCache() != null) {
398-
OkHttpClient clientNoCache = new OkHttpClient().setProxy(getProxy(host));
399-
gb.withConnector(new ForceValidationOkHttpConnector(client, clientNoCache));
400-
} else {
401-
gb.withConnector(new OkHttpConnector(new OkUrlFactory(client)));
402-
}
395+
gb.withConnector(new OkHttpConnector(new OkUrlFactory(client)));
403396

404397
if (username != null) {
405398
gb.withPassword(username, password);
@@ -639,36 +632,4 @@ public String toString() {
639632
}
640633

641634
}
642-
643-
/**
644-
* A {@link HttpConnector} that uses {@link OkHttpConnector} when caching is enabled.
645-
* Starts with the {@code Cache-Control} header configured to always revalidate requests
646-
* against the remote server using conditional GET requests.
647-
* Allows Jenkins to fallback to uncached query if requests fail due to flaky caching.
648-
*/
649-
@Restricted(NoExternalUse.class)
650-
/*package*/ static class ForceValidationOkHttpConnector extends OkHttpConnector {
651-
private static final String FORCE_VALIDATION = new CacheControl.Builder()
652-
.maxAge(0, TimeUnit.SECONDS)
653-
.build()
654-
.toString();
655-
private static final String HEADER_NAME = "Cache-Control";
656-
private final OkHttpConnector uncachedConnector;
657-
658-
public ForceValidationOkHttpConnector(OkHttpClient client, OkHttpClient uncachedClient) {
659-
super(new OkUrlFactory(client));
660-
this.uncachedConnector = new OkHttpConnector(new OkUrlFactory(uncachedClient));
661-
}
662-
663-
/*package*/ OkHttpConnector getUncachedConnector() {
664-
return uncachedConnector;
665-
}
666-
667-
@Override
668-
public HttpURLConnection connect(URL url) throws IOException {
669-
HttpURLConnection connection = super.connect(url);
670-
connection.setRequestProperty(HEADER_NAME, FORCE_VALIDATION);
671-
return connection;
672-
}
673-
}
674635
}

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMProbe.java‎

Lines changed: 4 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -36,25 +36,20 @@
3636
import org.eclipse.jgit.lib.Constants;
3737
import org.kohsuke.github.GHCommit;
3838
import org.kohsuke.github.GHContent;
39-
import org.kohsuke.github.GHFileNotFoundException;
4039
import org.kohsuke.github.GHRef;
4140
import org.kohsuke.github.GHRepository;
4241
import org.kohsuke.github.GitHub;
4342

43+
import java.io.FileNotFoundException;
4444
import java.io.IOException;
4545
import java.util.List;
46-
import java.util.Map;
47-
import java.util.Optional;
48-
import java.util.logging.Level;
4946
import java.util.logging.Logger;
5047

5148

5249
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
5350
class GitHubSCMProbe extends SCMProbe implements GitHubClosable {
5451
private static final long serialVersionUID = 1L;
5552
private static final Logger LOG = Logger.getLogger(GitHubSCMProbe.class.getName());
56-
static /*mostly final*/ boolean JENKINS_54126_WORKAROUND = Boolean.parseBoolean(System.getProperty(GitHubSCMProbe.class.getName() + ".JENKINS_54126_WORKAROUND", Boolean.TRUE.toString()));
57-
static /*mostly final*/ boolean STAT_RETHROW_API_FNF = Boolean.parseBoolean(System.getProperty(GitHubSCMProbe.class.getName() + ".STAT_RETHROW_API_FNF", Boolean.TRUE.toString()));
5853
private final SCMRevision revision;
5954
private final transient GitHub gitHub;
6055
private final transient GHRepository repo;
@@ -138,8 +133,8 @@ public long lastModified() {
138133
@Override
139134
public SCMProbeStat stat(@NonNull String path) throws IOException {
140135
checkOpen();
141-
int index = path.lastIndexOf('/') + 1;
142136
try {
137+
int index = path.lastIndexOf('/') + 1;
143138
List<GHContent> directoryContent = repo.getDirectoryContent(path.substring(0, index), Constants.R_REFS + ref);
144139
for (GHContent content : directoryContent) {
145140
if (content.getPath().equals(path)) {
@@ -159,50 +154,8 @@ public SCMProbeStat stat(@NonNull String path) throws IOException {
159154
return SCMProbeStat.fromAlternativePath(content.getPath());
160155
}
161156
}
162-
} catch (GHFileNotFoundException fnf) {
163-
boolean finicky = false;
164-
if (index == 0 || index == 1) {
165-
// the revision does not exist, we should complain unless JENKINS-54126.
166-
finicky = true;
167-
} else {
168-
try {
169-
repo.getDirectoryContent("/", Constants.R_REFS + ref);
170-
} catch (IOException e) {
171-
// this must be an issue with the revision, so complain unless JENKINS-54126
172-
fnf.addSuppressed(e);
173-
finicky = true;
174-
}
175-
// means that does not exist and this is handled below this try/catch block.
176-
}
177-
if (finicky && JENKINS_54126_WORKAROUND) {
178-
LOG.log(Level.FINE, String.format("JENKINS-54126 Received finicky response from GitHub %s : %s", repo.getFullName(), ref), fnf);
179-
final Optional<List<String>> status;
180-
final Map<String, List<String>> responseHeaderFields = fnf.getResponseHeaderFields();
181-
if (responseHeaderFields != null) {
182-
status = Optional.ofNullable(responseHeaderFields.get(null));
183-
} else {
184-
status = Optional.empty();
185-
}
186-
187-
if (GitHubSCMSource.getCacheSize() > 0
188-
&& gitHub.getConnector() instanceof Connector.ForceValidationOkHttpConnector
189-
&& status.isPresent() && status.get().stream().anyMatch((s) -> s.contains("40"))) { //Any status >= 400 is a FNF in okhttp
190-
//JENKINS-54126 try again without cache headers
191-
LOG.log(Level.FINE, "JENKINS-54126 Attempting the request again with workaround.");
192-
final Connector.ForceValidationOkHttpConnector oldConnector = (Connector.ForceValidationOkHttpConnector) gitHub.getConnector();
193-
try {
194-
//TODO I'm not sure we are alone in using this connector so maybe concurrent modification problems
195-
gitHub.setConnector(oldConnector.getUncachedConnector());
196-
return stat(path);
197-
} finally {
198-
gitHub.setConnector(oldConnector);
199-
}
200-
} else if (STAT_RETHROW_API_FNF){
201-
throw fnf;
202-
} else {
203-
LOG.log(Level.FINE, "JENKINS-54126 silently ignoring the problem.");
204-
}
205-
}
157+
} catch (FileNotFoundException fnf) {
158+
// means that does not exist and this is handled below this try/catch block.
206159
}
207160
return SCMProbeStat.fromType(SCMFile.Type.NONEXISTENT);
208161
}

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMProbeTest.java‎

Lines changed: 107 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
package org.jenkinsci.plugins.github_branch_source;
22

33
import com.github.tomakehurst.wiremock.core.WireMockConfiguration;
4+
import com.github.tomakehurst.wiremock.extension.responsetemplating.ResponseTemplateTransformer;
45
import com.github.tomakehurst.wiremock.http.RequestMethod;
56
import com.github.tomakehurst.wiremock.junit.WireMockRule;
67
import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
78
import jenkins.scm.api.SCMHeadOrigin;
89
import jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy;
10+
import org.apache.commons.io.FileUtils;
911
import org.junit.Before;
1012
import org.junit.Rule;
1113
import org.junit.Test;
@@ -14,7 +16,8 @@
1416
import org.kohsuke.github.GHRepository;
1517
import org.kohsuke.github.GitHub;
1618

17-
import java.io.FileNotFoundException;
19+
import java.io.File;
20+
import java.io.IOException;
1821

1922
import static org.junit.Assert.*;
2023
import static com.github.tomakehurst.wiremock.client.WireMock.*;
@@ -25,78 +28,136 @@ public class GitHubSCMProbeTest {
2528
public JenkinsRule j = new JenkinsRule();
2629
public static WireMockRuleFactory factory = new WireMockRuleFactory();
2730
@Rule
28-
public WireMockRule githubApi = factory.getRule(WireMockConfiguration.options().dynamicPort().usingFilesUnderClasspath("api"));
31+
public WireMockRule githubApi = factory.getRule(WireMockConfiguration.options()
32+
.dynamicPort().usingFilesUnderClasspath("cache_failure")
33+
.extensions(ResponseTemplateTransformer.builder().global(true).maxCacheEntries(0L).build()));
2934
private GitHubSCMProbe probe;
3035

3136
@Before
3237
public void setUp() throws Exception {
33-
GitHubSCMProbe.JENKINS_54126_WORKAROUND = true;
34-
GitHubSCMProbe.STAT_RETHROW_API_FNF = true;
35-
final GitHub github = Connector.connect("http://localhost:" + githubApi.port(), null);
38+
// Clear all caches before each test
39+
File cacheBaseDir = new File(j.jenkins.getRootDir(),
40+
GitHubSCMProbe.class.getName() + ".cache");
41+
if (cacheBaseDir.exists()) {
42+
FileUtils.cleanDirectory(cacheBaseDir);
43+
}
44+
3645
githubApi.stubFor(
37-
get(urlEqualTo("/repos/cloudbeers/yolo"))
38-
.willReturn(aResponse()
39-
.withStatus(200)
40-
.withHeader("Content-Type", "application/json")
41-
.withBodyFile("body-cloudbeers-yolo-PucD6.json"))
46+
get(urlEqualTo("/repos/cloudbeers/yolo"))
47+
.willReturn(aResponse()
48+
.withStatus(200)
49+
.withHeader("Content-Type", "application/json")
50+
.withBodyFile("body-cloudbeers-yolo-PucD6.json"))
4251
);
52+
}
53+
54+
void createProbeForPR(int number) throws IOException {
55+
final GitHub github = Connector.connect("http://localhost:" + githubApi.port(), null);
56+
4357
final GHRepository repo = github.getRepository("cloudbeers/yolo");
44-
final PullRequestSCMHead head = new PullRequestSCMHead("PR-1", "cloudbeers", "yolo", "b", 1, new BranchSCMHead("master"), new SCMHeadOrigin.Fork("rsandell"), ChangeRequestCheckoutStrategy.MERGE);
58+
final PullRequestSCMHead head = new PullRequestSCMHead("PR-" + number, "cloudbeers", "yolo", "b", number, new BranchSCMHead("master"), new SCMHeadOrigin.Fork("rsandell"), ChangeRequestCheckoutStrategy.MERGE);
4559
probe = new GitHubSCMProbe(github, repo,
46-
head,
47-
new PullRequestSCMRevision(head, "a", "b"));
60+
head,
61+
new PullRequestSCMRevision(head, "a", "b"));
4862
}
4963

5064
@Issue("JENKINS-54126")
51-
@Test(expected = FileNotFoundException.class)
65+
@Test
5266
public void statWhenRootIs404() throws Exception {
53-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).willReturn(aResponse().withStatus(404)));
54-
probe.stat("Jenkinsfile").exists();
55-
}
67+
githubApi.stubFor(get(urlEqualTo("/repos/cloudbeers/yolo/contents/?ref=refs%2Fpull%2F1%2Fmerge")).willReturn(aResponse().withStatus(404)).atPriority(0));
5668

57-
@Issue("JENKINS-54126")
58-
@Test
59-
public void statWhenRootIs404WorkaroundOff() throws Exception {
60-
GitHubSCMProbe.JENKINS_54126_WORKAROUND = false;
61-
GitHubSCMProbe.STAT_RETHROW_API_FNF = false;
62-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).willReturn(aResponse().withStatus(404)));
63-
assertFalse(probe.stat("README.md").exists());
64-
}
69+
createProbeForPR(1);
6570

66-
@Issue("JENKINS-54126")
67-
@Test(expected = FileNotFoundException.class)
68-
public void statWhenDirAndRootIs404() throws Exception {
69-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).willReturn(aResponse().withStatus(200)));
70-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/subdir")).willReturn(aResponse().withStatus(404)));
71-
probe.stat("subdir/Jenkinsfile").exists();
71+
assertFalse(probe.stat("Jenkinsfile").exists());
7272
}
7373

7474
@Issue("JENKINS-54126")
7575
@Test
7676
public void statWhenDirIs404() throws Exception {
77-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/subdir")).willReturn(aResponse().withStatus(404)));
78-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
79-
.willReturn(aResponse()
80-
.withStatus(200)
81-
.withHeader("Content-Type", "application/json")
82-
.withBodyFile("body-yolo-contents-8rd37.json")));
77+
githubApi.stubFor(get(urlEqualTo("/repos/cloudbeers/yolo/contents/subdir?ref=refs%2Fpull%2F1%2Fmerge")).willReturn(aResponse().withStatus(404)).atPriority(0));
78+
79+
createProbeForPR(1);
80+
81+
assertTrue(probe.stat("README.md").exists());
82+
assertFalse(probe.stat("subdir").exists());
8383
assertFalse(probe.stat("subdir/Jenkinsfile").exists());
8484
}
8585

8686
@Issue("JENKINS-54126")
8787
@Test
88-
public void statWhenRootIs404AndCacheOnThenOff() throws Exception {
88+
public void statWhenRoot404andThenIncorrectCached() throws Exception {
8989
GitHubSCMSource.setCacheSize(10);
90-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).withHeader("Cache-Control", containing("max-age")).willReturn(aResponse().withStatus(404)));
91-
githubApi.stubFor(get(urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).withHeader("Cache-Control", absent())
92-
.willReturn(aResponse()
93-
.withStatus(200)
94-
.withHeader("Content-Type", "application/json")
95-
.withBodyFile("body-yolo-contents-8rd37.json")));
9690

91+
createProbeForPR(9);
92+
93+
// JENKINS-54126 happens when:
94+
// 1. client asks for a resource "Z" that doesn't exist
95+
// ---> client receives a 404 response from github and caches it.
96+
// ---> Important: GitHub does not send ETag header for 404 responses.
97+
// 2. Resource "Z" gets created on GitHub but some means.
98+
// 3. client (eventually) asks for the resource "Z" again.
99+
// ---> Since the the client has a cached response without ETag, it sends "If-Modified-Since" header
100+
// ---> Resource has changed (it was created).
101+
//
102+
// ---> EXPECTED: GitHub should respond with 200 and data.
103+
// ---> ACTUAL: GitHub server lies, responds with incorrect 304 response, telling client that the cached data is still valid.
104+
// ---> THE BAD: Client cache believes GitHub - uses the previously cached 404 (and even adds the ETag).
105+
// ---> THE UGLY: Client is now stuck with a bad cached 404, and can't get rid of it until the resource is _updated_ again or the cache is cleared manually.
106+
//
107+
// This is the cause of JENKINS-54126. This is a pervasive GitHub server problem.
108+
// We see it mostly in this one scenario, but it will happen anywhere the server returns a 404.
109+
// It cannot be reliably detected or mitigated at the level of this plugin.
110+
//
111+
// WORKAROUND (implemented in the github-api library):
112+
// 4. the github-api library recognizes any 404 with ETag as invalid. Does not return it to the client.
113+
// ---> The github-api library automatically retries the request with "no-cache" to force refresh with valid data.
114+
115+
// 1.
116+
assertFalse(probe.stat("README.md").exists());
117+
118+
// 3.
119+
// Without 4. this would return false and would stay false.
120+
assertTrue(probe.stat("README.md").exists());
121+
122+
// 5. Verify caching is working
97123
assertTrue(probe.stat("README.md").exists());
98-
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).withHeader("Cache-Control", containing("max-age")));
99-
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/")).withHeader("Cache-Control", absent()));
124+
125+
// Verify the expected requests were made
126+
if(hudson.Functions.isWindows()) {
127+
// On windows caching is disabled by default, so the work around doesn't happen
128+
githubApi.verify(3, RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
129+
.withHeader("Cache-Control", equalTo("max-age=0"))
130+
.withHeader("If-Modified-Since", absent())
131+
.withHeader("If-None-Match", absent())
132+
);
133+
} else {
134+
// 1.
135+
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
136+
.withHeader("Cache-Control", equalTo("max-age=0"))
137+
.withHeader("If-None-Match", absent())
138+
.withHeader("If-Modified-Since", absent())
139+
);
140+
141+
// 3.
142+
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
143+
.withHeader("Cache-Control", containing("max-age"))
144+
.withHeader("If-None-Match", absent())
145+
.withHeader("If-Modified-Since", containing("GMT"))
146+
);
147+
148+
// 4.
149+
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
150+
.withHeader("Cache-Control", equalTo("no-cache"))
151+
.withHeader("If-Modified-Since", absent())
152+
.withHeader("If-None-Match", absent())
153+
);
154+
155+
// 5.
156+
githubApi.verify(RequestPatternBuilder.newRequestPattern(RequestMethod.GET, urlPathEqualTo("/repos/cloudbeers/yolo/contents/"))
157+
.withHeader("Cache-Control", equalTo("max-age=0"))
158+
.withHeader("If-None-Match", equalTo("\"d3be5b35b8d84ef7ac03c0cc9c94ed81\""))
159+
);
160+
}
100161
}
101162

102163
}

0 commit comments

Comments
 (0)