only create new client when config changed

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/http/HttpClientHolder.java

@@ -0,0 +1,70 @@
+package rocks.inspectit.ocelot.core.config.propertysources.http;
+
+import org.apache.http.client.HttpClient;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import rocks.inspectit.ocelot.config.model.config.HttpConfigSettings;
+
+import java.io.IOException;
+import java.time.Duration;
+
+/**
+ * Stores an instance of a {@link CloseableHttpClient} and it's relevant {@link HttpConfigSettings}.
+ * Since the HTTP client is a rather expensive object, we create one instance and recreate it only
+ * if the settings have changed.
+ */
+public class HttpClientHolder {
+
+    private CloseableHttpClient httpClient;
+
+    private Duration connectionTimeout;
+
+    private Duration connectionRequestTimeout;
+
+    private Duration socketTimeout;
+
+    /**
+     * Returns a {@link HttpClient}, which is used for fetching the configuration.
+     * If the HTTP settings changed, a new client will be created and the old one will be closed.
+     *
+     * @return a {@link HttpClient} instance.
+     */
+    public CloseableHttpClient getHttpClient(HttpConfigSettings httpSettings) throws IOException {
+        if(isUpdated(httpSettings) || httpClient == null) {
+            RequestConfig.Builder configBuilder = RequestConfig.custom();
+
+            if (httpSettings.getConnectionTimeout() != null) {
+                int connectionTimeout = (int) httpSettings.getConnectionTimeout().toMillis();
+                configBuilder = configBuilder.setConnectTimeout(connectionTimeout);
+            }
+            if (httpSettings.getConnectionRequestTimeout() != null) {
+                int connectionRequestTimeout = (int) httpSettings.getConnectionRequestTimeout().toMillis();
+                configBuilder = configBuilder.setConnectionRequestTimeout(connectionRequestTimeout);
+            }
+            if (httpSettings.getSocketTimeout() != null) {
+                int socketTimeout = (int) httpSettings.getSocketTimeout().toMillis();
+                configBuilder = configBuilder.setSocketTimeout(socketTimeout);
+            }
+
+            RequestConfig config = configBuilder.build();
+
+            if (httpClient != null) httpClient.close();
+            httpClient = HttpClientBuilder.create().setDefaultRequestConfig(config).build();
+
+            connectionTimeout = httpSettings.getConnectionTimeout();
+            connectionRequestTimeout = httpSettings.getConnectionRequestTimeout();
+            socketTimeout = httpSettings.getSocketTimeout();
+        }
+        return httpClient;
+    }
+
+    /**
+     * @return true, if the provided settings differ from the active settings
+     */
+    private boolean isUpdated(HttpConfigSettings settings) {
+        return settings.getConnectionTimeout() != connectionTimeout ||
+                settings.getConnectionRequestTimeout() != connectionRequestTimeout ||
+                settings.getSocketTimeout() != socketTimeout;
+    }
+}

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/http/HttpPropertySourceState.java

@@ -13,11 +13,9 @@
 import org.apache.http.HttpStatus;
 import org.apache.http.client.ClientProtocolException;
 import org.apache.http.client.HttpClient;
-import org.apache.http.client.config.RequestConfig;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.utils.URIBuilder;
 import org.apache.http.impl.client.CloseableHttpClient;
-import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.http.util.EntityUtils;
 import org.springframework.core.env.PropertiesPropertySource;
 import org.springframework.core.env.PropertySource;
@@ -64,6 +62,11 @@ public class HttpPropertySourceState {
     @Getter
     private final String name;
 
+    /**
+     * The holder of the HTTP client.
+     */
+    private final HttpClientHolder clientHolder;
+
     /**
      * The currently used settings.
      */
@@ -115,6 +118,7 @@ public class HttpPropertySourceState {
      */
     public HttpPropertySourceState(String name, HttpConfigSettings currentSettings) {
         this.name = name;
+        this.clientHolder = new HttpClientHolder();
         this.currentSettings = currentSettings;
         errorCounter = 0;
         //ensure that currentPropertySource is never null, even if the initial fetching fails
@@ -169,32 +173,6 @@ private Properties parseProperties(String rawProperties) throws InvalidPropertie
 
     }
 
-    /**
-     * Creates the {@link HttpClient} which is used for fetching the configuration.
-     *
-     * @return A new {@link HttpClient} instance.
-     */
-    private CloseableHttpClient createHttpClient() {
-        RequestConfig.Builder configBuilder = RequestConfig.custom();
-
-        if (currentSettings.getConnectionTimeout() != null) {
-            int connectionTimeout = (int) currentSettings.getConnectionTimeout().toMillis();
-            configBuilder = configBuilder.setConnectTimeout(connectionTimeout);
-        }
-        if (currentSettings.getConnectionRequestTimeout() != null) {
-            int connectionRequestTimeout = (int) currentSettings.getConnectionRequestTimeout().toMillis();
-            configBuilder = configBuilder.setConnectionRequestTimeout(connectionRequestTimeout);
-        }
-        if (currentSettings.getSocketTimeout() != null) {
-            int socketTimeout = (int) currentSettings.getSocketTimeout().toMillis();
-            configBuilder = configBuilder.setSocketTimeout(socketTimeout);
-        }
-
-        RequestConfig config = configBuilder.build();
-
-        return HttpClientBuilder.create().setDefaultRequestConfig(config).build();
-    }
-
     private String fetchConfiguration(boolean fallBackToFile) {
         HttpGet httpGet;
         try {
@@ -206,10 +184,9 @@ private String fetchConfiguration(boolean fallBackToFile) {
 
         String configuration = null;
         boolean isError = true;
-        // We create the httpClient outside the retry as it is a rather expensive object, but we create it for each call
-        // again, because the configuration may have changed. The "but" is currently pure speculation. Ideally we would
-        // recreate it only if configuration has changed.
-        try (CloseableHttpClient httpClient = createHttpClient()) {
+
+        try {
+            CloseableHttpClient httpClient = clientHolder.getHttpClient(currentSettings);
             Retry retry;
             if (fallBackToFile) {
                 // fallBackToFile == true means the agent started.

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/config/propertysources/http/HttpClientHolderTest.java

@@ -0,0 +1,67 @@
+package rocks.inspectit.ocelot.core.config.propertysources.http;
+
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.junit.jupiter.api.Test;
+import rocks.inspectit.ocelot.config.model.config.HttpConfigSettings;
+
+import java.io.IOException;
+import java.time.Duration;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class HttpClientHolderTest {
+
+    @Test
+    void shouldNotCreateNewHttpClientWhenConfigIsSame() throws IOException {
+        HttpClientHolder holder = new HttpClientHolder();
+        HttpConfigSettings settings = new HttpConfigSettings();
+
+        settings.setConnectionTimeout(Duration.ofSeconds(30));
+        CloseableHttpClient client = holder.getHttpClient(settings);
+        CloseableHttpClient anotherClient = holder.getHttpClient(settings);
+
+        assertThat(client).isSameAs(anotherClient);
+    }
+
+    @Test
+    void shouldCreateNewHttpClientWhenConnectionTimeoutChanges() throws IOException {
+        HttpClientHolder holder = new HttpClientHolder();
+        HttpConfigSettings settings = new HttpConfigSettings();
+
+        settings.setConnectionTimeout(Duration.ofSeconds(30));
+        CloseableHttpClient client = holder.getHttpClient(settings);
+
+        settings.setConnectionTimeout(Duration.ofSeconds(10));
+        CloseableHttpClient updatedClient = holder.getHttpClient(settings);
+
+        assertThat(updatedClient).isNotSameAs(client);
+    }
+
+    @Test
+    void shouldCreateNewHttpClientWhenConnectionRequestTimeoutChanges() throws IOException {
+        HttpClientHolder holder = new HttpClientHolder();
+        HttpConfigSettings settings = new HttpConfigSettings();
+
+        settings.setConnectionRequestTimeout(Duration.ofSeconds(30));
+        CloseableHttpClient client = holder.getHttpClient(settings);
+
+        settings.setConnectionRequestTimeout(Duration.ofSeconds(10));
+        CloseableHttpClient updatedClient = holder.getHttpClient(settings);
+
+        assertThat(updatedClient).isNotSameAs(client);
+    }
+
+    @Test
+    void shouldCreateNewHttpClientWhenSocketTimeoutChanges() throws IOException {
+        HttpClientHolder holder = new HttpClientHolder();
+        HttpConfigSettings settings = new HttpConfigSettings();
+
+        settings.setSocketTimeout(Duration.ofSeconds(30));
+        CloseableHttpClient client = holder.getHttpClient(settings);
+
+        settings.setSocketTimeout(Duration.ofSeconds(10));
+        CloseableHttpClient updatedClient = holder.getHttpClient(settings);
+
+        assertThat(updatedClient).isNotSameAs(client);
+    }
+}