Skip to content

Commit 612800b

Browse files
bitwisemanbitwiseman
authored
Merge pull request #843 from bitwiseman/task/body-close
[JENKINS-62655] Ensure connection response stream is always closed
2 parents 873c93a + a6bbb1d commit 612800b

File tree

11 files changed

+249
-43
lines changed

11 files changed

+249
-43
lines changed

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import com.fasterxml.jackson.annotation.JacksonInject;
44
import com.fasterxml.jackson.annotation.JsonProperty;
5-
import org.jetbrains.annotations.NotNull;
65

76
import java.io.IOException;
87
import java.net.URL;
@@ -158,7 +157,6 @@ public void delete() throws IOException {
158157
team.root.createRequest().method("DELETE").setRawUrlPath(getRawUrlPath(team, number)).send();
159158
}
160159

161-
@NotNull
162160
private static String getRawUrlPath(@Nonnull GHTeam team, @CheckForNull Long discussionNumber) {
163161
return team.getUrl().toString() + "/discussions" + (discussionNumber == null ? "" : "/" + discussionNumber);
164162
}

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

Lines changed: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import com.fasterxml.jackson.databind.ObjectWriter;
99
import com.fasterxml.jackson.databind.PropertyNamingStrategy;
1010
import com.fasterxml.jackson.databind.introspect.VisibilityChecker;
11+
import org.apache.commons.io.IOUtils;
1112
import org.apache.commons.lang3.StringUtils;
1213

1314
import java.io.FileNotFoundException;
@@ -351,39 +352,43 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Gi
351352

352353
GitHubResponse.ResponseInfo responseInfo = null;
353354
try {
354-
if (LOGGER.isLoggable(FINE)) {
355-
LOGGER.log(FINE,
356-
"GitHub API request [" + (login == null ? "anonymous" : login) + "]: " + request.method()
357-
+ " " + request.url().toString());
355+
try {
356+
if (LOGGER.isLoggable(FINE)) {
357+
LOGGER.log(FINE,
358+
"GitHub API request [" + (login == null ? "anonymous" : login) + "]: "
359+
+ request.method() + " " + request.url().toString());
360+
}
361+
362+
rateLimitChecker.checkRateLimit(this, request);
363+
364+
responseInfo = getResponseInfo(request);
365+
noteRateLimit(responseInfo);
366+
detectOTPRequired(responseInfo);
367+
368+
if (isInvalidCached404Response(responseInfo)) {
369+
// Setting "Cache-Control" to "no-cache" stops the cache from supplying
370+
// "If-Modified-Since" or "If-None-Match" values.
371+
// This makes GitHub give us current data (not incorrectly cached data)
372+
request = request.toBuilder().withHeader("Cache-Control", "no-cache").build();
373+
continue;
374+
}
375+
if (!(isRateLimitResponse(responseInfo) || isAbuseLimitResponse(responseInfo))) {
376+
return createResponse(responseInfo, handler);
377+
}
378+
} catch (IOException e) {
379+
// For transient errors, retry
380+
if (retryConnectionError(e, request.url(), retries)) {
381+
continue;
382+
}
383+
384+
throw interpretApiError(e, request, responseInfo);
358385
}
359386

360-
rateLimitChecker.checkRateLimit(this, request);
361-
362-
responseInfo = getResponseInfo(request);
363-
noteRateLimit(responseInfo);
364-
detectOTPRequired(responseInfo);
365-
366-
if (isInvalidCached404Response(responseInfo)) {
367-
// Setting "Cache-Control" to "no-cache" stops the cache from supplying
368-
// "If-Modified-Since" or "If-None-Match" values.
369-
// This makes GitHub give us current data (not incorrectly cached data)
370-
request = request.toBuilder().withHeader("Cache-Control", "no-cache").build();
371-
continue;
372-
}
373-
if (!(isRateLimitResponse(responseInfo) || isAbuseLimitResponse(responseInfo))) {
374-
return createResponse(responseInfo, handler);
375-
}
376-
} catch (IOException e) {
377-
// For transient errors, retry
378-
if (retryConnectionError(e, request.url(), retries)) {
379-
continue;
380-
}
381-
382-
throw interpretApiError(e, request, responseInfo);
387+
handleLimitingErrors(responseInfo);
388+
} finally {
389+
IOUtils.closeQuietly(responseInfo);
383390
}
384391

385-
handleLimitingErrors(responseInfo);
386-
387392
} while (--retries >= 0);
388393

389394
throw new GHIOException("Ran out of retries for URL: " + request.url().toString());

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,10 @@ private InputStream wrapStream(InputStream stream) throws IOException {
235235

236236
private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());
237237

238+
@Override
239+
public void close() throws IOException {
240+
IOUtils.closeQuietly(connection.getInputStream());
241+
}
238242
}
239243

240244
}

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import com.fasterxml.jackson.databind.JsonMappingException;
66
import org.apache.commons.io.IOUtils;
77

8+
import java.io.Closeable;
89
import java.io.IOException;
910
import java.io.InputStream;
1011
import java.io.InputStreamReader;
@@ -212,7 +213,7 @@ interface BodyHandler<T> {
212213
* Initial response information supplied to a {@link BodyHandler} when a response is initially received and before
213214
* the body is processed.
214215
*/
215-
static abstract class ResponseInfo {
216+
static abstract class ResponseInfo implements Closeable {
216217

217218
private static final Comparator<String> nullableCaseInsensitiveComparator = Comparator
218219
.nullsFirst(String.CASE_INSENSITIVE_ORDER);
@@ -317,12 +318,8 @@ public Map<String, List<String>> headers() {
317318
@Nonnull
318319
String getBodyAsString() throws IOException {
319320
InputStreamReader r = null;
320-
try {
321-
r = new InputStreamReader(this.bodyStream(), StandardCharsets.UTF_8);
322-
return IOUtils.toString(r);
323-
} finally {
324-
IOUtils.closeQuietly(r);
325-
}
321+
r = new InputStreamReader(this.bodyStream(), StandardCharsets.UTF_8);
322+
return IOUtils.toString(r);
326323
}
327324
}
328325

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
*/
2424
package org.kohsuke.github;
2525

26+
import org.apache.commons.io.IOUtils;
27+
28+
import java.io.ByteArrayInputStream;
2629
import java.io.IOException;
2730
import java.io.InputStream;
2831
import java.net.MalformedURLException;
@@ -108,7 +111,10 @@ public int fetchHttpStatusCode() throws IOException {
108111
* the io exception
109112
*/
110113
public InputStream fetchStream() throws IOException {
111-
return client.sendRequest(this, (responseInfo) -> responseInfo.bodyStream()).body();
114+
return client
115+
.sendRequest(this,
116+
(responseInfo) -> new ByteArrayInputStream(IOUtils.toByteArray(responseInfo.bodyStream())))
117+
.body();
112118
}
113119

114120
/**

‎src/test/java/org/kohsuke/github/GHIssueEventAttributeTest.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package org.kohsuke.github;
22

3-
import org.jetbrains.annotations.NotNull;
43
import org.junit.Test;
54

65
import java.io.IOException;
@@ -41,7 +40,6 @@ public void accept(final GHIssueEvent event) {
4140
}
4241
}
4342

44-
@NotNull
4543
private List<GHIssueEvent> listEvents(final Type type) throws IOException {
4644
return StreamSupport
4745
.stream(gitHub.getRepository("chids/project-milestone-test").getIssue(1).listEvents().spliterator(),

‎src/test/java/org/kohsuke/github/RequesterRetryTest.java‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,13 @@
22

33
import com.github.tomakehurst.wiremock.http.Fault;
44
import com.github.tomakehurst.wiremock.stubbing.Scenario;
5+
import okhttp3.ConnectionPool;
6+
import okhttp3.OkHttpClient;
57
import org.junit.Before;
8+
import org.junit.Ignore;
69
import org.junit.Test;
710
import org.kohsuke.github.extras.ImpatientHttpConnector;
11+
import org.kohsuke.github.extras.okhttp3.OkHttpConnector;
812
import org.mockito.Mockito;
913

1014
import java.io.ByteArrayOutputStream;
@@ -20,6 +24,7 @@
2024
import java.security.Permission;
2125
import java.util.List;
2226
import java.util.Map;
27+
import java.util.concurrent.TimeUnit;
2328
import java.util.logging.Logger;
2429
import java.util.logging.SimpleFormatter;
2530
import java.util.logging.StreamHandler;
@@ -57,7 +62,8 @@ private GHRepository getRepository(GitHub gitHub) throws IOException {
5762
public void attachLogCapturer() {
5863
logCapturingStream = new ByteArrayOutputStream();
5964
customLogHandler = new StreamHandler(logCapturingStream, new SimpleFormatter());
60-
log.addHandler(customLogHandler);
65+
Logger.getLogger(GitHubClient.class.getName()).addHandler(customLogHandler);
66+
Logger.getLogger(OkHttpClient.class.getName()).addHandler(customLogHandler);
6167
}
6268

6369
public String getTestCapturedLog() throws IOException {
@@ -66,11 +72,40 @@ public String getTestCapturedLog() throws IOException {
6672
}
6773

6874
public void resetTestCapturedLog() throws IOException {
69-
log.removeHandler(customLogHandler);
75+
Logger.getLogger(GitHubClient.class.getName()).removeHandler(customLogHandler);
76+
Logger.getLogger(OkHttpClient.class.getName()).removeHandler(customLogHandler);
7077
customLogHandler.close();
7178
attachLogCapturer();
7279
}
7380

81+
@Ignore("Used okhttp3 and this to verify connection closing. To variable for CI system.")
82+
@Test
83+
public void testGitHubIsApiUrlValid() throws Exception {
84+
85+
OkHttpClient client = new OkHttpClient().newBuilder()
86+
.connectionPool(new ConnectionPool(2, 100, TimeUnit.MILLISECONDS))
87+
.build();
88+
89+
OkHttpConnector connector = new OkHttpConnector(client);
90+
91+
for (int x = 0; x < 100; x++) {
92+
93+
this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
94+
.withConnector(connector)
95+
.build();
96+
97+
try {
98+
gitHub.checkApiUrlValidity();
99+
} catch (IOException ioe) {
100+
assertTrue(ioe.getMessage().contains("private mode enabled"));
101+
}
102+
Thread.sleep(100);
103+
}
104+
105+
String capturedLog = getTestCapturedLog();
106+
assertThat(capturedLog, not(containsString("leaked")));
107+
}
108+
74109
// Issue #539
75110
@Test
76111
public void testSocketConnectionAndRetry() throws Exception {

‎src/test/resources/org/kohsuke/github/RequesterRetryTest/wiremock/testGitHubIsApiUrlValid/__files/-2.json‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
{
2+
"current_user_url": "https://api.github.com/user",
3+
"current_user_authorizations_html_url": "https://github.com/settings/connections/applications{/client_id}",
4+
"authorizations_url": "https://api.github.com/authorizations",
5+
"code_search_url": "https://api.github.com/search/code?q={query}{&page,per_page,sort,order}",
6+
"commit_search_url": "https://api.github.com/search/commits?q={query}{&page,per_page,sort,order}",
7+
"emails_url": "https://api.github.com/user/emails",
8+
"emojis_url": "https://api.github.com/emojis",
9+
"events_url": "https://api.github.com/events",
10+
"feeds_url": "https://api.github.com/feeds",
11+
"followers_url": "https://api.github.com/user/followers",
12+
"following_url": "https://api.github.com/user/following{/target}",
13+
"gists_url": "https://api.github.com/gists{/gist_id}",
14+
"hub_url": "https://api.github.com/hub",
15+
"issue_search_url": "https://api.github.com/search/issues?q={query}{&page,per_page,sort,order}",
16+
"issues_url": "https://api.github.com/issues",
17+
"keys_url": "https://api.github.com/user/keys",
18+
"label_search_url": "https://api.github.com/search/labels?q={query}&repository_id={repository_id}{&page,per_page}",
19+
"notifications_url": "https://api.github.com/notifications",
20+
"organization_url": "https://api.github.com/orgs/{org}",
21+
"organization_repositories_url": "https://api.github.com/orgs/{org}/repos{?type,page,per_page,sort}",
22+
"organization_teams_url": "https://api.github.com/orgs/{org}/teams",
23+
"public_gists_url": "https://api.github.com/gists/public",
24+
"rate_limit_url": "https://api.github.com/rate_limit",
25+
"repository_url": "https://api.github.com/repos/{owner}/{repo}",
26+
"repository_search_url": "https://api.github.com/search/repositories?q={query}{&page,per_page,sort,order}",
27+
"current_user_repositories_url": "https://api.github.com/user/repos{?type,page,per_page,sort}",
28+
"starred_url": "https://api.github.com/user/starred{/owner}{/repo}",
29+
"starred_gists_url": "https://api.github.com/gists/starred",
30+
"user_url": "https://api.github.com/users/{user}",
31+
"user_organizations_url": "https://api.github.com/user/orgs",
32+
"user_repositories_url": "https://api.github.com/users/{user}/repos{?type,page,per_page,sort}",
33+
"user_search_url": "https://api.github.com/search/users?q={query}{&page,per_page,sort,order}"
34+
}

‎src/test/resources/org/kohsuke/github/RequesterRetryTest/wiremock/testGitHubIsApiUrlValid/__files/user-1.json‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
{
2+
"login": "bitwiseman",
3+
"id": 1958953,
4+
"node_id": "MDQ6VXNlcjE5NTg5NTM=",
5+
"avatar_url": "https://avatars3.githubusercontent.com/u/1958953?v=4",
6+
"gravatar_id": "",
7+
"url": "https://api.github.com/users/bitwiseman",
8+
"html_url": "https://github.com/bitwiseman",
9+
"followers_url": "https://api.github.com/users/bitwiseman/followers",
10+
"following_url": "https://api.github.com/users/bitwiseman/following{/other_user}",
11+
"gists_url": "https://api.github.com/users/bitwiseman/gists{/gist_id}",
12+
"starred_url": "https://api.github.com/users/bitwiseman/starred{/owner}{/repo}",
13+
"subscriptions_url": "https://api.github.com/users/bitwiseman/subscriptions",
14+
"organizations_url": "https://api.github.com/users/bitwiseman/orgs",
15+
"repos_url": "https://api.github.com/users/bitwiseman/repos",
16+
"events_url": "https://api.github.com/users/bitwiseman/events{/privacy}",
17+
"received_events_url": "https://api.github.com/users/bitwiseman/received_events",
18+
"type": "User",
19+
"site_admin": false,
20+
"name": "Liam Newman",
21+
"company": "Cloudbees, Inc.",
22+
"blog": "",
23+
"location": "Seattle, WA, USA",
24+
"email": "[email protected]",
25+
"hireable": null,
26+
"bio": "https://twitter.com/bitwiseman",
27+
"twitter_username": null,
28+
"public_repos": 187,
29+
"public_gists": 7,
30+
"followers": 161,
31+
"following": 9,
32+
"created_at": "2012-07-11T20:38:33Z",
33+
"updated_at": "2020-05-29T18:24:44Z",
34+
"private_gists": 19,
35+
"total_private_repos": 12,
36+
"owned_private_repos": 0,
37+
"disk_usage": 33700,
38+
"collaborators": 0,
39+
"two_factor_authentication": true,
40+
"plan": {
41+
"name": "free",
42+
"space": 976562499,
43+
"collaborators": 0,
44+
"private_repos": 10000
45+
}
46+
}

‎src/test/resources/org/kohsuke/github/RequesterRetryTest/wiremock/testGitHubIsApiUrlValid/mappings/-2.json‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
{
2+
"id": "e4a8f577-6cb7-4106-84f2-7ee0205b76ce",
3+
"name": "",
4+
"request": {
5+
"url": "/",
6+
"method": "GET"
7+
},
8+
"response": {
9+
"status": 200,
10+
"bodyFileName": "-2.json",
11+
"headers": {
12+
"Date": "Wed, 10 Jun 2020 23:28:05 GMT",
13+
"Content-Type": "application/json; charset=utf-8",
14+
"Server": "GitHub.com",
15+
"Status": "200 OK",
16+
"X-RateLimit-Limit": "5000",
17+
"X-RateLimit-Remaining": "4988",
18+
"X-RateLimit-Reset": "1591835235",
19+
"Cache-Control": "private, max-age=60, s-maxage=60",
20+
"Vary": [
21+
"Accept, Authorization, Cookie, X-GitHub-OTP",
22+
"Accept-Encoding, Accept, X-Requested-With",
23+
"Accept-Encoding"
24+
],
25+
"ETag": "W/\"e01e638f43d5e359ea8768a81a8aa145\"",
26+
"X-OAuth-Scopes": "admin:org, admin:org_hook, admin:public_key, admin:repo_hook, delete_repo, gist, notifications, repo, user, write:discussion",
27+
"X-Accepted-OAuth-Scopes": "",
28+
"X-GitHub-Media-Type": "github.v3; format=json",
29+
"Strict-Transport-Security": "max-age=31536000; includeSubdomains; preload",
30+
"X-Frame-Options": "deny",
31+
"X-Content-Type-Options": "nosniff",
32+
"X-XSS-Protection": "1; mode=block",
33+
"Referrer-Policy": "origin-when-cross-origin, strict-origin-when-cross-origin",
34+
"Content-Security-Policy": "default-src 'none'",
35+
"X-GitHub-Request-Id": "F658:22C1:47B4:5D7E:5EE16C85"
36+
}
37+
},
38+
"uuid": "e4a8f577-6cb7-4106-84f2-7ee0205b76ce",
39+
"persistent": true,
40+
"insertionIndex": 2
41+
}

0 commit comments

Comments
 (0)