Merge pull request #843 from bitwiseman/task/body-close

[JENKINS-62655] Ensure connection response stream is always closed

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

@@ -2,7 +2,6 @@
 
 import com.fasterxml.jackson.annotation.JacksonInject;
 import com.fasterxml.jackson.annotation.JsonProperty;
-import org.jetbrains.annotations.NotNull;
 
 import java.io.IOException;
 import java.net.URL;
@@ -158,7 +157,6 @@ public void delete() throws IOException {
         team.root.createRequest().method("DELETE").setRawUrlPath(getRawUrlPath(team, number)).send();
     }
 
-    @NotNull
     private static String getRawUrlPath(@Nonnull GHTeam team, @CheckForNull Long discussionNumber) {
         return team.getUrl().toString() + "/discussions" + (discussionNumber == null ? "" : "/" + discussionNumber);
     }

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

@@ -8,6 +8,7 @@
 import com.fasterxml.jackson.databind.ObjectWriter;
 import com.fasterxml.jackson.databind.PropertyNamingStrategy;
 import com.fasterxml.jackson.databind.introspect.VisibilityChecker;
+import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang3.StringUtils;
 
 import java.io.FileNotFoundException;
@@ -351,39 +352,43 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Gi
 
             GitHubResponse.ResponseInfo responseInfo = null;
             try {
-                if (LOGGER.isLoggable(FINE)) {
-                    LOGGER.log(FINE,
-                            "GitHub API request [" + (login == null ? "anonymous" : login) + "]: " + request.method()
-                                    + " " + request.url().toString());
+                try {
+                    if (LOGGER.isLoggable(FINE)) {
+                        LOGGER.log(FINE,
+                                "GitHub API request [" + (login == null ? "anonymous" : login) + "]: "
+                                        + request.method() + " " + request.url().toString());
+                    }
+
+                    rateLimitChecker.checkRateLimit(this, request);
+
+                    responseInfo = getResponseInfo(request);
+                    noteRateLimit(responseInfo);
+                    detectOTPRequired(responseInfo);
+
+                    if (isInvalidCached404Response(responseInfo)) {
+                        // Setting "Cache-Control" to "no-cache" stops the cache from supplying
+                        // "If-Modified-Since" or "If-None-Match" values.
+                        // This makes GitHub give us current data (not incorrectly cached data)
+                        request = request.toBuilder().withHeader("Cache-Control", "no-cache").build();
+                        continue;
+                    }
+                    if (!(isRateLimitResponse(responseInfo) || isAbuseLimitResponse(responseInfo))) {
+                        return createResponse(responseInfo, handler);
+                    }
+                } catch (IOException e) {
+                    // For transient errors, retry
+                    if (retryConnectionError(e, request.url(), retries)) {
+                        continue;
+                    }
+
+                    throw interpretApiError(e, request, responseInfo);
                 }
 
-                rateLimitChecker.checkRateLimit(this, request);
-
-                responseInfo = getResponseInfo(request);
-                noteRateLimit(responseInfo);
-                detectOTPRequired(responseInfo);
-
-                if (isInvalidCached404Response(responseInfo)) {
-                    // Setting "Cache-Control" to "no-cache" stops the cache from supplying
-                    // "If-Modified-Since" or "If-None-Match" values.
-                    // This makes GitHub give us current data (not incorrectly cached data)
-                    request = request.toBuilder().withHeader("Cache-Control", "no-cache").build();
-                    continue;
-                }
-                if (!(isRateLimitResponse(responseInfo) || isAbuseLimitResponse(responseInfo))) {
-                    return createResponse(responseInfo, handler);
-                }
-            } catch (IOException e) {
-                // For transient errors, retry
-                if (retryConnectionError(e, request.url(), retries)) {
-                    continue;
-                }
-
-                throw interpretApiError(e, request, responseInfo);
+                handleLimitingErrors(responseInfo);
+            } finally {
+                IOUtils.closeQuietly(responseInfo);
             }
 
-            handleLimitingErrors(responseInfo);
-
         } while (--retries >= 0);
 
         throw new GHIOException("Ran out of retries for URL: " + request.url().toString());

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

@@ -235,6 +235,10 @@ private InputStream wrapStream(InputStream stream) throws IOException {
 
         private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());
 
+        @Override
+        public void close() throws IOException {
+            IOUtils.closeQuietly(connection.getInputStream());
+        }
     }
 
 }

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

@@ -5,6 +5,7 @@
 import com.fasterxml.jackson.databind.JsonMappingException;
 import org.apache.commons.io.IOUtils;
 
+import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -212,7 +213,7 @@ interface BodyHandler<T> {
      * Initial response information supplied to a {@link BodyHandler} when a response is initially received and before
      * the body is processed.
      */
-    static abstract class ResponseInfo {
+    static abstract class ResponseInfo implements Closeable {
 
         private static final Comparator<String> nullableCaseInsensitiveComparator = Comparator
                 .nullsFirst(String.CASE_INSENSITIVE_ORDER);
@@ -317,12 +318,8 @@ public Map<String, List<String>> headers() {
         @Nonnull
         String getBodyAsString() throws IOException {
             InputStreamReader r = null;
-            try {
-                r = new InputStreamReader(this.bodyStream(), StandardCharsets.UTF_8);
-                return IOUtils.toString(r);
-            } finally {
-                IOUtils.closeQuietly(r);
-            }
+            r = new InputStreamReader(this.bodyStream(), StandardCharsets.UTF_8);
+            return IOUtils.toString(r);
         }
     }
 

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

@@ -23,6 +23,9 @@
  */
 package org.kohsuke.github;
 
+import org.apache.commons.io.IOUtils;
+
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.MalformedURLException;
@@ -108,7 +111,10 @@ public int fetchHttpStatusCode() throws IOException {
      *             the io exception
      */
     public InputStream fetchStream() throws IOException {
-        return client.sendRequest(this, (responseInfo) -> responseInfo.bodyStream()).body();
+        return client
+                .sendRequest(this,
+                        (responseInfo) -> new ByteArrayInputStream(IOUtils.toByteArray(responseInfo.bodyStream())))
+                .body();
     }
 
     /**

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

@@ -1,6 +1,5 @@
 package org.kohsuke.github;
 
-import org.jetbrains.annotations.NotNull;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -41,7 +40,6 @@ public void accept(final GHIssueEvent event) {
         }
     }
 
-    @NotNull
     private List<GHIssueEvent> listEvents(final Type type) throws IOException {
         return StreamSupport
                 .stream(gitHub.getRepository("chids/project-milestone-test").getIssue(1).listEvents().spliterator(),

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

@@ -2,9 +2,13 @@
 
 import com.github.tomakehurst.wiremock.http.Fault;
 import com.github.tomakehurst.wiremock.stubbing.Scenario;
+import okhttp3.ConnectionPool;
+import okhttp3.OkHttpClient;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.kohsuke.github.extras.ImpatientHttpConnector;
+import org.kohsuke.github.extras.okhttp3.OkHttpConnector;
 import org.mockito.Mockito;
 
 import java.io.ByteArrayOutputStream;
@@ -20,6 +24,7 @@
 import java.security.Permission;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 import java.util.logging.SimpleFormatter;
 import java.util.logging.StreamHandler;
@@ -57,7 +62,8 @@ private GHRepository getRepository(GitHub gitHub) throws IOException {
     public void attachLogCapturer() {
         logCapturingStream = new ByteArrayOutputStream();
         customLogHandler = new StreamHandler(logCapturingStream, new SimpleFormatter());
-        log.addHandler(customLogHandler);
+        Logger.getLogger(GitHubClient.class.getName()).addHandler(customLogHandler);
+        Logger.getLogger(OkHttpClient.class.getName()).addHandler(customLogHandler);
     }
 
     public String getTestCapturedLog() throws IOException {
@@ -66,11 +72,40 @@ public String getTestCapturedLog() throws IOException {
     }
 
     public void resetTestCapturedLog() throws IOException {
-        log.removeHandler(customLogHandler);
+        Logger.getLogger(GitHubClient.class.getName()).removeHandler(customLogHandler);
+        Logger.getLogger(OkHttpClient.class.getName()).removeHandler(customLogHandler);
         customLogHandler.close();
         attachLogCapturer();
     }
 
+    @Ignore("Used okhttp3 and this to verify connection closing. To variable for CI system.")
+    @Test
+    public void testGitHubIsApiUrlValid() throws Exception {
+
+        OkHttpClient client = new OkHttpClient().newBuilder()
+                .connectionPool(new ConnectionPool(2, 100, TimeUnit.MILLISECONDS))
+                .build();
+
+        OkHttpConnector connector = new OkHttpConnector(client);
+
+        for (int x = 0; x < 100; x++) {
+
+            this.gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
+                    .withConnector(connector)
+                    .build();
+
+            try {
+                gitHub.checkApiUrlValidity();
+            } catch (IOException ioe) {
+                assertTrue(ioe.getMessage().contains("private mode enabled"));
+            }
+            Thread.sleep(100);
+        }
+
+        String capturedLog = getTestCapturedLog();
+        assertThat(capturedLog, not(containsString("leaked")));
+    }
+
     // Issue #539
     @Test
     public void testSocketConnectionAndRetry() throws Exception {

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

@@ -0,0 +1,34 @@
+{
+  "current_user_url": "https://api.github.com/user",
+  "current_user_authorizations_html_url": "https://github.com/settings/connections/applications{/client_id}",
+  "authorizations_url": "https://api.github.com/authorizations",
+  "code_search_url": "https://api.github.com/search/code?q={query}{&page,per_page,sort,order}",
+  "commit_search_url": "https://api.github.com/search/commits?q={query}{&page,per_page,sort,order}",
+  "emails_url": "https://api.github.com/user/emails",
+  "emojis_url": "https://api.github.com/emojis",
+  "events_url": "https://api.github.com/events",
+  "feeds_url": "https://api.github.com/feeds",
+  "followers_url": "https://api.github.com/user/followers",
+  "following_url": "https://api.github.com/user/following{/target}",
+  "gists_url": "https://api.github.com/gists{/gist_id}",
+  "hub_url": "https://api.github.com/hub",
+  "issue_search_url": "https://api.github.com/search/issues?q={query}{&page,per_page,sort,order}",
+  "issues_url": "https://api.github.com/issues",
+  "keys_url": "https://api.github.com/user/keys",
+  "label_search_url": "https://api.github.com/search/labels?q={query}&repository_id={repository_id}{&page,per_page}",
+  "notifications_url": "https://api.github.com/notifications",
+  "organization_url": "https://api.github.com/orgs/{org}",
+  "organization_repositories_url": "https://api.github.com/orgs/{org}/repos{?type,page,per_page,sort}",
+  "organization_teams_url": "https://api.github.com/orgs/{org}/teams",
+  "public_gists_url": "https://api.github.com/gists/public",
+  "rate_limit_url": "https://api.github.com/rate_limit",
+  "repository_url": "https://api.github.com/repos/{owner}/{repo}",
+  "repository_search_url": "https://api.github.com/search/repositories?q={query}{&page,per_page,sort,order}",
+  "current_user_repositories_url": "https://api.github.com/user/repos{?type,page,per_page,sort}",
+  "starred_url": "https://api.github.com/user/starred{/owner}{/repo}",
+  "starred_gists_url": "https://api.github.com/gists/starred",
+  "user_url": "https://api.github.com/users/{user}",
+  "user_organizations_url": "https://api.github.com/user/orgs",
+  "user_repositories_url": "https://api.github.com/users/{user}/repos{?type,page,per_page,sort}",
+  "user_search_url": "https://api.github.com/search/users?q={query}{&page,per_page,sort,order}"
+}

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

@@ -0,0 +1,46 @@
+{
+  "login": "bitwiseman",
+  "id": 1958953,
+  "node_id": "MDQ6VXNlcjE5NTg5NTM=",
+  "avatar_url": "https://avatars3.githubusercontent.com/u/1958953?v=4",
+  "gravatar_id": "",
+  "url": "https://api.github.com/users/bitwiseman",
+  "html_url": "https://github.com/bitwiseman",
+  "followers_url": "https://api.github.com/users/bitwiseman/followers",
+  "following_url": "https://api.github.com/users/bitwiseman/following{/other_user}",
+  "gists_url": "https://api.github.com/users/bitwiseman/gists{/gist_id}",
+  "starred_url": "https://api.github.com/users/bitwiseman/starred{/owner}{/repo}",
+  "subscriptions_url": "https://api.github.com/users/bitwiseman/subscriptions",
+  "organizations_url": "https://api.github.com/users/bitwiseman/orgs",
+  "repos_url": "https://api.github.com/users/bitwiseman/repos",
+  "events_url": "https://api.github.com/users/bitwiseman/events{/privacy}",
+  "received_events_url": "https://api.github.com/users/bitwiseman/received_events",
+  "type": "User",
+  "site_admin": false,
+  "name": "Liam Newman",
+  "company": "Cloudbees, Inc.",
+  "blog": "",
+  "location": "Seattle, WA, USA",
+  "email": "bitwiseman@gmail.com",
+  "hireable": null,
+  "bio": "https://twitter.com/bitwiseman",
+  "twitter_username": null,
+  "public_repos": 187,
+  "public_gists": 7,
+  "followers": 161,
+  "following": 9,
+  "created_at": "2012-07-11T20:38:33Z",
+  "updated_at": "2020-05-29T18:24:44Z",
+  "private_gists": 19,
+  "total_private_repos": 12,
+  "owned_private_repos": 0,
+  "disk_usage": 33700,
+  "collaborators": 0,
+  "two_factor_authentication": true,
+  "plan": {
+    "name": "free",
+    "space": 976562499,
+    "collaborators": 0,
+    "private_repos": 10000
+  }
+}

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

@@ -0,0 +1,41 @@
+{
+  "id": "e4a8f577-6cb7-4106-84f2-7ee0205b76ce",
+  "name": "",
+  "request": {
+    "url": "/",
+    "method": "GET"
+  },
+  "response": {
+    "status": 200,
+    "bodyFileName": "-2.json",
+    "headers": {
+      "Date": "Wed, 10 Jun 2020 23:28:05 GMT",
+      "Content-Type": "application/json; charset=utf-8",
+      "Server": "GitHub.com",
+      "Status": "200 OK",
+      "X-RateLimit-Limit": "5000",
+      "X-RateLimit-Remaining": "4988",
+      "X-RateLimit-Reset": "1591835235",
+      "Cache-Control": "private, max-age=60, s-maxage=60",
+      "Vary": [
+        "Accept, Authorization, Cookie, X-GitHub-OTP",
+        "Accept-Encoding, Accept, X-Requested-With",
+        "Accept-Encoding"
+      ],
+      "ETag": "W/\"e01e638f43d5e359ea8768a81a8aa145\"",
+      "X-OAuth-Scopes": "admin:org, admin:org_hook, admin:public_key, admin:repo_hook, delete_repo, gist, notifications, repo, user, write:discussion",
+      "X-Accepted-OAuth-Scopes": "",
+      "X-GitHub-Media-Type": "github.v3; format=json",
+      "Strict-Transport-Security": "max-age=31536000; includeSubdomains; preload",
+      "X-Frame-Options": "deny",
+      "X-Content-Type-Options": "nosniff",
+      "X-XSS-Protection": "1; mode=block",
+      "Referrer-Policy": "origin-when-cross-origin, strict-origin-when-cross-origin",
+      "Content-Security-Policy": "default-src 'none'",
+      "X-GitHub-Request-Id": "F658:22C1:47B4:5D7E:5EE16C85"
+    }
+  },
+  "uuid": "e4a8f577-6cb7-4106-84f2-7ee0205b76ce",
+  "persistent": true,
+  "insertionIndex": 2
+}