Skip to content

Remove http client leak #62

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 24, 2016

Conversation

i386
Copy link
Contributor

@i386 i386 commented Oct 21, 2016

The ClosableHttpClient is never released correctly if an exception occurs. This could lead to leaked unclosed connections to Github.

Note: this PR depends on #61 and needs a rebase when it is merged. As such it contains all the improvements in #62 and this PR.

PTAL @samrocketman

@@ -478,6 +403,33 @@ public HttpResponse doFinishLogin(StaplerRequest request)
return HttpResponses.redirectToContextRoot(); // referer should be always there, but be defensive
}

@Nullable
private String getAccessToken(@Nonnull String code) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method replaces the code above for resolving the code to an accessToken.

@samrocketman
Copy link
Member

Please rebase this. #61 has been merged.

@samrocketman
Copy link
Member

@i386 I'm also having trouble with the pom2.3 branch building with Java 1.8. Do you mind taking a look at it as well? The reason to migrate to pom2.3 is releasing via HTTPS.

@i386 i386 force-pushed the remove-http-client-leak branch from 47bc8cd to b4deecf Compare October 22, 2016 01:36
@i386
Copy link
Contributor Author

i386 commented Oct 22, 2016

@samrocketman I've rebased and pushed - this PR is ready for review.

Strange that you are having issues on Java 8 as thats what I've been building it with locally. What error are you seeing?

@samrocketman
Copy link
Member

samrocketman commented Oct 23, 2016

I'll start reviewing this PR and merge if I think it's ready.


in reply to:

Strange that you are having issues on Java 8 as thats what I've been building it with locally. What error are you seeing?

I'm using:

  • Oracle java version "1.8.0_91".
  • Apache Maven 3.3.9

On the pom2.3 branch I get errors like:

testCanReadAndBuildOrgRepositoryICollaborateOn(org.jenkinsci.plugins.GithubRequireOrganizationMembershipACLTest)  Time elapsed: 0.003 sec  <<< ERROR!
java.lang.IllegalStateException: attempt to register a second Permission for hudson.model.Item.ViewStatus
        at hudson.security.PermissionGroup.add(PermissionGroup.java:72)
        at hudson.security.Permission.<init>(Permission.java:162)
        at hudson.security.Permission.<init>(Permission.java:168)
        at org.jenkinsci.plugins.GithubRequireOrganizationMembershipACLTest.<init>(GithubRequireOrganizationMembershipACLTest.java:95)

So I guess it's not specifically related to Java 8 in this case but due to upgrading the base version of Jenkins the plugin is built against. I'm having trouble upgrading to the new Jenkins pom.xml format. You can learn about what I tried in the commit message 6a0aee1. There's also links I added as comments inline in the diff.

If you figure it out feel free to cherry-pick my commit and propose fixes in a new PR. Thanks for taking the time to entertain my request.

@samrocketman
Copy link
Member

Tested OK.

@samrocketman samrocketman merged commit e12ce1c into jenkinsci:master Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants