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

Ensure that connections are closed for error responses #346

Merged
merged 1 commit into from
Feb 27, 2017

Conversation

stephenc
Copy link
Contributor

  • This was endless fun to trace, but I found it at last. This should
    stop the WARNING: A connection to https://api.github.com/ was leaked. Did you forget to close a response body? messages in the logs when
    using the OkHttpConnector.

@reviewbybees

- This was endless fun to trace, but I found it at last. This should
stop the `WARNING: A connection to https://api.github.com/ was leaked.
Did you forget to close a response body?` messages in the logs when
using the OkHttpConnector.
@stephenc
Copy link
Contributor Author

No I do not have an automated test for this fix, the testing I have done is to use the old version and run some code that is known to cause the issue... wait maybe 5 minutes (can show as early as immediately, seems to be at worst every 5 minutes) and then you get the error in the logs

Then switch to the fix, rerun the same code (GHRepository.getCollaborators() with an anonymous connection is good) and then wait 15 minutes... no error logged!

Copy link
Collaborator

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Generally looks good to me 🐝 . The code in Requester is definitely misplaced

// ensure that the connection opened by getResponseCode gets closed
try {
IOUtils.closeQuietly(uc.getInputStream());
} catch (IOException ignore) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it;s a leftover from the previous fix. closeQuietlydoes not throw IOExceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uc.getInputStream() does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And when using the OkHttpConnector and the HTTP response code is >= 400 it will throw the IOE always (not that JVM's HttpURLConnection will do that)... given that this is the expected case for returning true, we need to access the error stream in order to ensure that the request is closed for the OkHttpConnector path... which we have to do using the error stream 🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KostyaSha
Copy link
Contributor

@stepehenc have you tried to run existing ITs?

@stephenc
Copy link
Contributor Author

@KostyaSha they fail for me before making changes, I do not expect them to magically pass after my changes, but I have done as extensive a testing as possible without the less than stellar automated tests

Copy link
Contributor

@KostyaSha KostyaSha left a comment

Choose a reason for hiding this comment

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

ok, let's try

@rsandell
Copy link

🐝

@stephenc
Copy link
Contributor Author

@reviewbybees done, @kohsuke can you merge and release?

@ghost
Copy link

ghost commented Feb 25, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

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.

6 participants