close responses, avoid leaking connections (#394)

pom.xml

@@ -161,7 +161,7 @@
         <dependency>
             <groupId>com.squareup.okhttp3</groupId>
             <artifactId>okhttp</artifactId>
-            <version>3.14.2</version>
+            <version>4.1.1</version>
         </dependency>
         <dependency>
             <groupId>org.yaml</groupId>

src/main/java/com/openshift/internal/restclient/ApiTypeMapper.java

@@ -39,6 +39,7 @@
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.Response;
+import okhttp3.ResponseBody;
 
 /**
  * Typemapper to determine the endpoints for various openshift resources
@@ -253,14 +254,19 @@ private String readEndpoint(final String endpoint) {
         try {
             final URL url = new URL(new URL(this.baseUrl), endpoint);
             LOGGER.debug(url.toString());
-            Request request = new Request.Builder().url(url).build();
-            Response response = client.newCall(request).execute();
-            return response.body().string();
+            return request(url);
         } catch (IOException e) {
             throw new OpenShiftException(e, "Unable to read endpoint %s/%s", this.baseUrl, endpoint);
         }
     }
 
+    private String request(final URL url) throws IOException {
+        Request request = new Request.Builder().url(url).build();
+        try (Response response = client.newCall(request).execute()) {
+            return response.body().string();
+        }
+    }
+
     static class ApiGroup implements IApiGroup {
         private final ModelNode node;
         private final String prefix;
@@ -322,7 +328,8 @@ public String getName() {
         @Override
         public Collection<String> getVersions() {
             return JBossDmrExtentions.get(getNode(), new HashMap<>(), "versions").asList().stream()
-                    .map(n -> n.asString()).collect(Collectors.toList());
+                    .map(ModelNode::asString)
+                    .collect(Collectors.toList());
         }
 
         @Override

src/main/java/com/openshift/internal/restclient/DefaultClient.java

@@ -290,23 +290,34 @@ private <T> T execute(ITypeFactory factory, String method, String kind, String v
             throw new UnsupportedOperationException("Generic create operation not supported for resource type 'List'");
         }
 
-        final URL endpoint = new URLBuilder(this.baseUrl, typeMapper).apiVersion(version).kind(kind).name(name)
-                .namespace(namespace).subresource(subresource).subContext(subContext).addParameters(params).build();
-
+        final URL endpoint = new URLBuilder(this.baseUrl, typeMapper)
+                .apiVersion(version)
+                .kind(kind)
+                .name(name)
+                .namespace(namespace)
+                .subresource(subresource)
+                .subContext(subContext)
+                .addParameters(params)
+                .build();
+        Request request = newRequestBuilderTo(endpoint.toString())
+                .method(method, requestBody)
+                .build();
+        LOGGER.debug("About to make {} request: {}", request.method(), request);
         try {
-            Request request = newRequestBuilderTo(endpoint.toString())
-                    .method(method, requestBody)
-                    .build();
-            LOGGER.debug("About to make {} request: {}", request.method(), request);
-            try (Response result = client.newCall(request).execute()) {
-                String response = result.body().string();
-                LOGGER.debug("Response: {}", response);
-                return (T) factory.createInstanceFrom(response);
-            }
+            String body = request(request);
+            return (T) factory.createInstanceFrom(body);
         } catch (IOException e) {
             throw new OpenShiftException(e, "Unable to execute request to %s", endpoint);
         }
     }
+
+    private String request(Request request) throws IOException {
+        try (Response response = client.newCall(request).execute()) {
+            String body = response.body().string() ;
+            LOGGER.debug("Response: {}", body);
+            return body;
+        }
+    }
 
     private String getApiVersion(JSONSerializeable payload) {
         String apiVersion = null;
@@ -358,9 +369,7 @@ public String getServerReadyStatus() {
                     .url(new URL(this.baseUrl, URL_HEALTH_CHECK))
                     .header(PROPERTY_ACCEPT, "*/*")
                     .build();
-            try (Response response = client.newCall(request).execute()) {
-                return response.body().string();
-            }
+            return request(request);
         } catch (IOException e) {
             throw new OpenShiftException(e,
                     "Exception while trying to determine the health/ready response of the server");

src/main/java/com/openshift/internal/restclient/capability/resources/DockerRegistryImageStreamImportCapability.java

@@ -11,6 +11,7 @@
 
 package com.openshift.internal.restclient.capability.resources;
 
+import java.io.IOException;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
@@ -41,6 +42,7 @@
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import okhttp3.Response;
+import okhttp3.ResponseBody;
 
 /**
  * Retrieve metadata directly from docker.
@@ -81,11 +83,12 @@ public String getName() {
     private boolean registryExists(OkHttpClient client) throws Exception {
         Request req = new Request.Builder().url(DEFAULT_DOCKER_REGISTRY)
                 .header(ResponseCodeInterceptor.X_OPENSHIFT_IGNORE_RCI, "true").build();
-        Response response = client.newCall(req).execute();
-        if (response == null) {
-            return false;
+        try (Response response = client.newCall(req).execute()) {
+            if (response == null) {
+                return false;
+            }
+            return (response.code() == STATUS_UNAUTHORIZED || response.code() == STATUS_OK);
         }
-        return (response.code() == STATUS_UNAUTHORIZED || response.code() == STATUS_OK);
     }
 
     /**
@@ -96,26 +99,34 @@ private String retrieveAuthToken(OkHttpClient client, String details) throws Exc
             Map<String, String> auth = parseAuthDetails(details);
             if (auth.containsKey(REALM)) {
                 Request request = createAuthRequest(auth);
-                Response response = client.newCall(request).execute();
-                LOG.debug("Auth response: " + response.toString());
-                if (response.code() == STATUS_OK
-                        && MEDIATYPE_APPLICATION_JSON.equals(response.headers().get(PROPERTY_CONTENT_TYPE))) {
-                    ModelNode tokenNode = ModelNode.fromJSONString(response.body().string());
-                    if (tokenNode.hasDefined(TOKEN)) {
-                        return tokenNode.get(TOKEN).asString();
-                    } else {
-                        LOG.debug("No auth token was found on auth response: " + tokenNode.toJSONString(false));
-                    }
+                return retrieveAuthToken(client, request);
+            } else {
+                LOG.info("Unable to retrieve authentication token - 'realm' was not found in the authenticate header: {}",
+                        auth.toString());
+            }
+        }
+        return null;
+    }
+
+    private String retrieveAuthToken(OkHttpClient client, Request request) throws IOException {
+        String token = null;
+        try (Response response = client.newCall(request).execute();
+                ResponseBody responseBody = response.body()) {
+            LOG.debug("Auth response: {}", responseBody.string());
+            if (response.code() == STATUS_OK
+                    && MEDIATYPE_APPLICATION_JSON.equals(response.headers().get(PROPERTY_CONTENT_TYPE))) {
+                ModelNode tokenNode = ModelNode.fromJSONString(responseBody.string());
+                if (tokenNode.hasDefined(TOKEN)) {
+                    token = tokenNode.get(TOKEN).asString();
                 } else {
-                    LOG.info(
-                            "Unable to retrieve authentication token as response was not OK and/or unexpected content type");
+                    LOG.debug("No auth token was found on auth response: {}", tokenNode.toJSONString(false));
                 }
             } else {
-                LOG.info("Unable to retrieve authentication token - 'realm' was not found in the authenticate header: "
-                        + auth.toString());
+                LOG.info(
+                        "Unable to retrieve authentication token as response was not OK and/or unexpected content type");
             }
         }
-        return null;
+        return token;
     }
 
     private Request createAuthRequest(Map<String, String> authParams) {
@@ -127,12 +138,12 @@ private Request createAuthRequest(Map<String, String> authParams) {
         }
         Request request = new Request.Builder().url(builder.build())
                 .header(ResponseCodeInterceptor.X_OPENSHIFT_IGNORE_RCI, "true").build();
-        LOG.debug("Auth request uri: " + request.url());
+        LOG.debug("Auth request uri: {}", request.url());
         return request;
     }
 
     private Map<String, String> parseAuthDetails(String auth) {
-        LOG.debug("Auth details header: " + auth);
+        LOG.debug("Auth details header: {}", auth);
         Map<String, String> map = new HashMap<>();
         String[] authAndValues = auth.split(" ");
         if (authAndValues.length == 2 && AUTHORIZATION_BEARER.equals(authAndValues[0])) {
@@ -156,18 +167,21 @@ private DockerResponse retrieveMetaData(OkHttpClient client, String token, Docke
             builder.header(PROPERTY_AUTHORIZATION, String.format("%s %s", AUTHORIZATION_BEARER, token));
 
         }
-        LOG.debug("retrieveMetaData uri: " + regUri);
-        Response response = client.newCall(builder.build()).execute();
-        LOG.debug("retrieveMetaData response: " + response.toString());
-        switch (response.code()) {
-        case STATUS_OK:
-            return new DockerResponse(DockerResponse.DATA, response.body().string());
-        case STATUS_UNAUTHORIZED:
-            return new DockerResponse(DockerResponse.AUTH,
-                    response.headers().get(IHttpConstants.PROPERTY_WWW_AUTHENTICATE));
+        LOG.debug("retrieveMetaData uri: {}", regUri);
+        try (Response response = client.newCall(builder.build()).execute();
+                ResponseBody responseBody = response.body()) {
+            LOG.debug("retrieveMetaData response: {}", responseBody.string());
+            switch (response.code()) {
+            case STATUS_OK:
+                return new DockerResponse(DockerResponse.DATA, responseBody.string());
+            case STATUS_UNAUTHORIZED:
+                return new DockerResponse(DockerResponse.AUTH,
+                        response.headers().get(IHttpConstants.PROPERTY_WWW_AUTHENTICATE));
+            default:
+                LOG.info("Unable to retrieve docker meta data: {}", responseBody.string());
+                return null;
+            }
         }
-        LOG.info("Unable to retrieve docker meta data: " + response.toString());
-        return null;
     }
 
     private static class DockerResponse {

src/main/java/com/openshift/internal/restclient/okhttp/OpenShiftAuthenticator.java

@@ -55,13 +55,17 @@ public class OpenShiftAuthenticator implements Authenticator, IHttpConstants {
     public Request authenticate(Route route, Response response) throws IOException {
         if (unauthorizedForCluster(response)) {
             String requestUrl = response.request().url().toString();
-            Request authRequest = new Request.Builder().addHeader(CSRF_TOKEN, "1").url(new URL(client.getAuthorizationEndpoint().toExternalForm()
-                    + "?response_type=token&client_id=openshift-challenging-client").toString()).build();
+            Request authRequest = new Request.Builder()
+                    .addHeader(CSRF_TOKEN, "1")
+                    .url(new URL(client.getAuthorizationEndpoint().toExternalForm() 
+                            + "?response_type=token&client_id=openshift-challenging-client").toString()).build();
             try (Response authResponse = tryAuth(authRequest)) {
                 if (authResponse.isSuccessful()) {
                     String token = extractAndSetAuthContextToken(authResponse);
-                    return response.request().newBuilder().header(IHttpConstants.PROPERTY_AUTHORIZATION,
-                            String.format("%s %s", IHttpConstants.AUTHORIZATION_BEARER, token)).build();
+                    return response.request().newBuilder()
+                            .header(IHttpConstants.PROPERTY_AUTHORIZATION,
+                                    String.format("%s %s", IHttpConstants.AUTHORIZATION_BEARER, token))
+                            .build();
                 }
             }
             throw new UnauthorizedException(captureAuthDetails(requestUrl),
@@ -78,24 +82,31 @@ private boolean unauthorizedForCluster(Response response) {
     }
 
     private Response tryAuth(Request authRequest) throws IOException {
-        return okClient.newBuilder().authenticator(new Authenticator() {
-
-            @Override
-            public Request authenticate(Route route, Response response) throws IOException {
-                if (StringUtils.isNotBlank(response.request().header(AUTH_ATTEMPTS))) {
-                    return null;
-                }
-                if (StringUtils.isNotBlank(response.header(IHttpConstants.PROPERTY_WWW_AUTHENTICATE))) {
-                    for (IChallangeHandler challangeHandler : challangeHandlers) {
-                        if (!challangeHandler.canHandle(response.headers())) {
-                            Builder requestBuilder = response.request().newBuilder().header(AUTH_ATTEMPTS, "1");
-                            return challangeHandler.handleChallange(requestBuilder).build();
+        return okClient.newBuilder()
+                .authenticator(new Authenticator() {
+
+                    @Override
+                    public Request authenticate(Route route, Response response) throws IOException {
+                        if (StringUtils.isNotBlank(response.request().header(AUTH_ATTEMPTS))) {
+                            return null;
+                        }
+                        if (StringUtils.isNotBlank(response.header(IHttpConstants.PROPERTY_WWW_AUTHENTICATE))) {
+                            for (IChallangeHandler challangeHandler : challangeHandlers) {
+                                if (!challangeHandler.canHandle(response.headers())) {
+                                    Builder requestBuilder = response.request().newBuilder()
+                                            .header(AUTH_ATTEMPTS, "1");
+                                    response.close();
+                                    return challangeHandler.handleChallange(requestBuilder).build();
+                                }
+                            }
                         }
+                        return null;
                     }
-                }
-                return null;
-            }
-        }).followRedirects(false).followRedirects(false).build().newCall(authRequest).execute();
+                })
+                .followRedirects(false)
+                .build()
+                .newCall(authRequest)
+                .execute();
     }
 
     private IAuthorizationDetails captureAuthDetails(String url) {

src/main/java/com/openshift/internal/restclient/okhttp/ResponseCodeInterceptor.java

@@ -62,22 +62,27 @@ public Response intercept(Chain chain) throws IOException {
                 response = makeSuccessIfAuthorized(response);
                 break;
             default:
-                if (response.request().tag() instanceof Ignore == false) {
+                if (!(response.request().tag() instanceof Ignore)) {
                     throw createOpenShiftException(client, response, null);
                 }
             }
         }
         return response;
     }
 
-    private Response makeSuccessIfAuthorized(Response response) {
+    private Response makeSuccessIfAuthorized(final Response response) {
+        Response returnedResponse = response;
         String location = response.header(PROPERTY_LOCATION);
         if (StringUtils.isNotBlank(location)
                 && URIUtils.splitFragment(location).containsKey(OpenShiftAuthenticator.ACCESS_TOKEN)) {
-            response = response.newBuilder().request(response.request()).code(STATUS_OK).headers(response.headers())
+            returnedResponse = response.newBuilder()
+                    .request(response.request())
+                    .code(STATUS_OK)
+                    .headers(response.headers())
                     .build();
+            response.close();
         }
-        return response;
+        return returnedResponse;
     }
 
     public void setClient(DefaultClient client) {

src/main/java/com/openshift/restclient/ClientBuilder.java

@@ -238,18 +238,25 @@ public IClient build() {
             // if we need to really expose them.
             dispatcher.setMaxRequests(maxRequests);
             dispatcher.setMaxRequestsPerHost(maxRequestsPerHost);
-            String[] pieces = { this.userAgentPrefix, "openshift-restclient-java", Version.userAgent() };
+            String[] pieces = { this.userAgentPrefix, "openshift-restclient-java", Version.userAgent };
             String userAgent = StringUtils.join(pieces, "/");
 
-            OkHttpClient.Builder builder = new OkHttpClient.Builder().addInterceptor(responseCodeInterceptor)
+            OkHttpClient.Builder builder = new OkHttpClient.Builder()
+                    .addInterceptor(responseCodeInterceptor)
                     .addNetworkInterceptor(new Interceptor() {
                         @Override
                         public Response intercept(Chain chain) throws IOException {
-                            Request agent = chain.request().newBuilder().header("User-Agent", userAgent).build();
+                            Request agent = chain.request().newBuilder()
+                                    .header("User-Agent", userAgent)
+                                    .build();
                             return chain.proceed(agent);
                         }
-                    }).authenticator(authenticator).dispatcher(dispatcher).readTimeout(readTimeout, readTimeoutUnit)
-                    .writeTimeout(writeTimeout, writeTimeoutUnit).connectTimeout(connectTimeout, connectTimeoutUnit)
+                    })
+                    .authenticator(authenticator)
+                    .dispatcher(dispatcher)
+                    .readTimeout(readTimeout, readTimeoutUnit)
+                    .writeTimeout(writeTimeout, writeTimeoutUnit)
+                    .connectTimeout(connectTimeout, connectTimeoutUnit)
                     .pingInterval(pingInterval, pingIntervalUnit)
                     .sslSocketFactory(sslContext.getSocketFactory(), trustManager);