Skip to content

Remove http client leak#62

Merged
samrocketman merged 1 commit into
jenkinsci:masterfrom
i386:remove-http-client-leak
Oct 24, 2016
Merged

Remove http client leak#62
samrocketman merged 1 commit into
jenkinsci:masterfrom
i386:remove-http-client-leak

Conversation

@i386
Copy link
Copy Markdown
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

Copy link
Copy Markdown
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
Copy Markdown
Member

Please rebase this. #61 has been merged.

@samrocketman
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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