Skip to content

Commit

Permalink
Ensure that connections are closed for error responses
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
stephenc committed Feb 23, 2017
1 parent 1f4325e commit 2627dc5
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 23 deletions.
17 changes: 13 additions & 4 deletions src/main/java/org/kohsuke/github/GitHub.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,12 @@
import java.util.Map;
import java.util.Set;
import java.util.TimeZone;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.apache.commons.codec.Charsets;
import org.apache.commons.codec.binary.Base64;
import org.apache.commons.io.IOUtils;

import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY;
import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE;
Expand Down Expand Up @@ -703,8 +702,18 @@ private boolean isPrivateModeEnabled() {
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Content-Type-Options: nosniff
*/
return uc.getResponseCode() == HTTP_UNAUTHORIZED
&& uc.getHeaderField("X-GitHub-Media-Type") != null;
try {
return uc.getResponseCode() == HTTP_UNAUTHORIZED
&& uc.getHeaderField("X-GitHub-Media-Type") != null;
} finally {
// ensure that the connection opened by getResponseCode gets closed
try {
IOUtils.closeQuietly(uc.getInputStream());
} catch (IOException ignore) {
// ignore
}
IOUtils.closeQuietly(uc.getErrorStream());
}
} catch (IOException e) {
return false;
}
Expand Down
38 changes: 19 additions & 19 deletions src/main/java/org/kohsuke/github/Requester.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
import java.util.Locale;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -642,6 +641,24 @@ private InputStream wrapStream(InputStream in) throws IOException {
" handling exception " + e, e);
throw e;
}
InputStream es = wrapStream(uc.getErrorStream());

This comment has been minimized.

Copy link
@atanasenko

atanasenko Jun 29, 2017

Won't be closed if wrapStream throws something

if (es != null) {
try {
String error = IOUtils.toString(es, "UTF-8");
if (e instanceof FileNotFoundException) {
// pass through 404 Not Found to allow the caller to handle it intelligently
e = (IOException) new FileNotFoundException(error).initCause(e);
} else if (e instanceof HttpException) {
HttpException http = (HttpException) e;
e = new HttpException(error, http.getResponseCode(), http.getResponseMessage(),
http.getUrl(), e);
} else {
e = (IOException) new IOException(error).initCause(e);
}
} finally {
IOUtils.closeQuietly(es);
}
}
if (responseCode == HttpURLConnection.HTTP_UNAUTHORIZED) // 401 / Unauthorized == bad creds
throw e;

Expand All @@ -657,24 +674,7 @@ private InputStream wrapStream(InputStream in) throws IOException {
return;
}

InputStream es = wrapStream(uc.getErrorStream());
try {
if (es!=null) {
String error = IOUtils.toString(es, "UTF-8");
if (e instanceof FileNotFoundException) {
// pass through 404 Not Found to allow the caller to handle it intelligently
throw (IOException) new FileNotFoundException(error).initCause(e);
} else if (e instanceof HttpException) {
HttpException http = (HttpException) e;
throw (IOException) new HttpException(error, http.getResponseCode(), http.getResponseMessage(), http.getUrl(), e);
} else {
throw (IOException) new IOException(error).initCause(e);
}
} else
throw e;
} finally {
IOUtils.closeQuietly(es);
}
throw e;
}

private static final List<String> METHODS_WITHOUT_BODY = asList("GET", "DELETE");
Expand Down

0 comments on commit 2627dc5

Please sign in to comment.