-
Notifications
You must be signed in to change notification settings - Fork 165
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
Conversation
@@ -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 { |
There was a problem hiding this comment.
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.
Please rebase this. #61 has been merged. |
@i386 I'm also having trouble with the |
47bc8cd
to
b4deecf
Compare
@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? |
I'll start reviewing this PR and merge if I think it's ready. in reply to:
I'm using:
On the
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. |
Tested OK. |
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