Skip to content
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

Assure the connection get closed after check #375

Merged
merged 1 commit into from Aug 30, 2018

Conversation

Projects
None yet
3 participants
@eryksz
Copy link

commented Aug 28, 2018

ClosableHttpResponse is not being closed if there is no Exception.
Moved the corresponding code to the finally block.

@dpursehouse

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

Hi @rsandell can you have a look at this? We've found that there are a lot of unclosed connections against Gerrit and this might be one of the causes.

@rsandell
Copy link
Member

left a comment

Though if possible it should be converted to a try with resource expression instead now that we have java 7 as the language level.

@rsandell rsandell merged commit 39519d0 into jenkinsci:master Aug 30, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@dpursehouse

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

@dpursehouse

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

I checked again and the animal-sniffer-maven-plugin is configured as:

                <configuration>
                    <signature>
                        <groupId>org.codehaus.mojo.signature</groupId>
                        <artifactId>java16</artifactId>
                        <version>1.1</version>
                    </signature>
                </configuration>

and the README still says "Java 6 runtime compatible", so I don't think we can use try-with-resource as it's not Java 6 compatible.

Or is the README and pom.xml out of date?

@rsandell

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

Oh, my bad mixing up plugins again :) I thought I had bumped the core dependency to 2.1x but apparently not :)

@eryksz eryksz deleted the eryksz:close-connection branch Aug 30, 2018

@dpursehouse

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2018

I thought I had bumped the core dependency to 2.1x

Isn't it about time to do this and move to Java 7 anyway? Or better, to Java 8? :)

BTW when is the next release (containing this fix) likely to appear?

@rsandell

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

Yes, it is definitely time. Should take a look at the statistics to see what core version to bump to. IIRC anything newer than 2.11 would imply Java 8.

I'm going through the low hanging fruits for merge and then release this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.