chore: Fix unexpected connection-pool shut-down of HttpClient4

cloudplatform/cloudplatform-connectivity/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpDestination.java

@@ -19,6 +19,7 @@
 import javax.annotation.Nullable;
 import javax.net.ssl.SSLContext;
 
+import lombok.val;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 
@@ -511,8 +512,7 @@ public static Builder fromDestination( @Nonnull final Destination destination )
             .getPropertyNames()
             .forEach(propertyName -> builder.property(propertyName, destination.get(propertyName).get()));
 
-        if( destination instanceof DefaultHttpDestination ) {
-            final DefaultHttpDestination httpDestination = (DefaultHttpDestination) destination;
+        if( destination instanceof DefaultHttpDestination httpDestination ) {
             builder.headers(httpDestination.customHeaders);
             builder
                 .headerProviders(httpDestination.getCustomHeaderProviders().toArray(new DestinationHeaderProvider[0]));
@@ -536,25 +536,47 @@ public boolean equals( @Nullable final Object o )
         }
 
         final DefaultHttpDestination that = (DefaultHttpDestination) o;
-        return new EqualsBuilder()
-            .append(baseProperties, that.baseProperties)
-            .append(customHeaders, that.customHeaders)
-            .append(
-                resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()),
-                resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull()))
-            .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore))
-            .isEquals();
+
+        if( headerProvidersFromClassLoading.size() != that.headerProvidersFromClassLoading.size()
+            || customHeaderProviders.size() != that.customHeaderProviders.size() ) {
+            return false;
+        }
+
+        val builder =
+            new EqualsBuilder()
+                .append(baseProperties, that.baseProperties)
+                .append(customHeaders, that.customHeaders)
+                .append(
+                    resolveCertificatesOnly(keyStoreSupplier.get().getOrNull()),
+                    resolveCertificatesOnly(that.keyStoreSupplier.get().getOrNull()))
+                .append(resolveCertificatesOnly(trustStore), resolveCertificatesOnly(that.trustStore));
+
+        customHeaderProviders
+            .forEach(
+                provider -> builder
+                    .append(provider, that.customHeaderProviders.get(customHeaderProviders.indexOf(provider))));
+        headerProvidersFromClassLoading
+            .forEach(
+                provider -> builder
+                    .append(
+                        provider,
+                        that.headerProvidersFromClassLoading.get(headerProvidersFromClassLoading.indexOf(provider))));
+        return builder.isEquals();
     }
 
     @Override
     public int hashCode()
     {
-        return new HashCodeBuilder(17, 37)
-            .append(baseProperties)
-            .append(customHeaders)
-            .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull()))
-            .append(resolveKeyStoreHashCode(trustStore))
-            .toHashCode();
+        val builder =
+            new HashCodeBuilder(17, 37)
+                .append(baseProperties)
+                .append(customHeaders)
+                .append(resolveKeyStoreHashCode(keyStoreSupplier.get().getOrNull()))
+                .append(resolveKeyStoreHashCode(trustStore));
+
+        customHeaderProviders.forEach(builder::append);
+        headerProvidersFromClassLoading.forEach(builder::append);
+        return builder.toHashCode();
     }
 
     /**

cloudplatform/connectivity-apache-httpclient4/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/HttpClientWrapper.java

@@ -93,10 +93,7 @@ public String toString()
 
     HttpClientWrapper withDestination( final HttpDestinationProperties destination )
     {
-        // explicitly check the reference equality, since equals doesn't check header providers
-        // this is a slight improvement, avoiding unnecessary wrapper instantiation
-        // in cases where destination objects are reused / served from cache
-        if( !destination.equals(this.destination) ) {
+        if( !destination.getUri().equals(this.destination.getUri()) ) {
             throw new ShouldNotHappenException(
                 "This method must not be used outside of updating an instance of HttpClientWrapper for http clients served from the HttpClientCache.");
         }

cloudplatform/connectivity-apache-httpclient4/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DefaultHttpClientCacheTest.java

@@ -1,5 +1,12 @@
 package com.sap.cloud.sdk.cloudplatform.connectivity;
 
+import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
+import static com.github.tomakehurst.wiremock.client.WireMock.equalTo;
+import static com.github.tomakehurst.wiremock.client.WireMock.get;
+import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.ok;
+import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
+import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
@@ -12,9 +19,11 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 
+import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.methods.HttpUriRequest;
+import org.jspecify.annotations.NonNull;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -28,9 +37,15 @@
 import com.sap.cloud.sdk.cloudplatform.tenant.Tenant;
 import com.sap.cloud.sdk.testutil.TestContext;
 
+import lombok.SneakyThrows;
+
 @Isolated
 class DefaultHttpClientCacheTest
 {
+    @RegisterExtension
+    static final WireMockExtension WIRE_MOCK_SERVER =
+        WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
+
     private static final HttpDestination DESTINATION = DefaultHttpDestination.builder("https://url1").build();
     private static final DefaultHttpDestination USER_TOKEN_EXCHANGE_DESTINATION =
         DefaultHttpDestination
@@ -357,6 +372,93 @@ void testInvalidatePrincipalCacheEntriesWithUserTokenExchangeDestination()
         assertThat(unclearedClientWithoutDestination).isSameAs(sut.tryGetHttpClient(FACTORY).get());
     }
 
+    @Test
+    @SneakyThrows
+    void testCachedEqualHttpClientsClosingBehavior()
+    {
+        WIRE_MOCK_SERVER.stubFor(get(anyUrl()).willReturn(ok()));
+
+        final DefaultHttpDestination destination1 =
+            DefaultHttpDestination
+                .builder(WIRE_MOCK_SERVER.baseUrl())
+                .headerProviders(c -> List.of(new Header("Authorization", "Bearer old")))
+                .build();
+        final DefaultHttpDestination destination2 =
+            DefaultHttpDestination
+                .builder(WIRE_MOCK_SERVER.baseUrl())
+                .headerProviders(c -> List.of(new Header("Authorization", "Bearer new")))
+                .build();
+
+        final HttpClient client1 = sut.tryGetHttpClient(destination1, FACTORY).get();
+        assertThat(((HttpClientWrapper) client1).getDestination()).isSameAs(destination1);
+        final HttpClient client2 = sut.tryGetHttpClient(destination2, FACTORY).get();
+        assertThat(((HttpClientWrapper) client2).getDestination()).isSameAs(destination2);
+
+        // Note: Destinations are now equal even with different header providers, since header providers
+        // are not part of the equality check. However, they are different instances, so they result in
+        // different HTTP clients due to the cache key being based on instance identity.
+        assertThat(destination1).isNotSameAs(destination2);
+
+        // Http clients are distinct instances, since the cache key contains the destination reference and not its content
+        assertThat(client1).isNotSameAs(client2);
+
+        // When using the exact same destination object, the same http-client wrapper should be returned
+        final HttpClient client1Again = sut.tryGetHttpClient(destination1, FACTORY).get();
+        assertThat(client1Again).isSameAs(client1);
+        assertThat(((HttpClientWrapper) client1Again).getDestination()).isSameAs(destination1);
+
+        // simulate garbage collection on client1
+        ((HttpClientWrapper) client1).close();
+
+        // since client1 did not inherit client2 connection manager, client2 is not shut down
+        client2.execute(new HttpGet());
+        WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()));
+    }
+
+    @Test
+    @SneakyThrows
+    void testCachedDestinationIsReused()
+    {
+        WIRE_MOCK_SERVER
+            .stubFor(
+                get(anyUrl())
+                    .withHeader("Authorization", equalTo("Bearer token1"))
+                    .inScenario("Refreshing token")
+                    .whenScenarioStateIs(STARTED)
+                    .willReturn(ok())
+                    .willSetStateTo("First token sent"));
+        WIRE_MOCK_SERVER
+            .stubFor(
+                get(anyUrl())
+                    .withHeader("Authorization", equalTo("Bearer token2"))
+                    .inScenario("Refreshing token")
+                    .whenScenarioStateIs("First token sent")
+                    .willReturn(ok()));
+
+        final DefaultHttpDestination destination =
+            DefaultHttpDestination.builder(WIRE_MOCK_SERVER.baseUrl()).headerProviders(c -> getHeaders()).build();
+
+        // token1 is sent
+        final HttpClient client1 = sut.tryGetHttpClient(destination, FACTORY).get();
+        client1.execute(new HttpGet());
+        WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token1")));
+
+        // token2 is sent
+        final HttpClient client2 = sut.tryGetHttpClient(destination, FACTORY).get();
+        client2.execute(new HttpGet());
+        WIRE_MOCK_SERVER.verify(1, getRequestedFor(anyUrl()).withHeader("Authorization", equalTo("Bearer token2")));
+
+        // Because the destination is cached, the same client is reused
+        assertThat(client1).isSameAs(client2);
+    }
+
+    private int count = 1;
+
+    private @NonNull List<Header> getHeaders()
+    {
+        return List.of(new Header("Authorization", "Bearer token" + count++));
+    }
+
     @Test
     void testPrincipalPropagationIsPrincipalIsolated()
     {

cloudplatform/connectivity-apache-httpclient5/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/ApacheHttpClient5Wrapper.java

@@ -78,10 +78,7 @@ public void close( final CloseMode closeMode )
 
     ApacheHttpClient5Wrapper withDestination( final HttpDestinationProperties destination )
     {
-        // explicitly check the reference equality, since equals doesn't check header providers
-        // this is a slight improvement, avoiding unnecessary wrapper instantiation
-        // in cases where destination objects are reused / served from cache
-        if( !destination.equals(this.destination) ) {
+        if( !destination.getUri().equals(this.destination.getUri()) ) {
             throw new ShouldNotHappenException(
                 "This method must not be used outside of updating an instance of ApacheHttpClient5Wrapper for http clients served from the ApacheHttpClient5Cache.");
         }