chore: Fix unexpected connection-pool shut-down

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

@@ -43,6 +43,7 @@
 import lombok.experimental.Accessors;
 import lombok.experimental.Delegate;
 import lombok.extern.slf4j.Slf4j;
+import lombok.val;
 
 /**
  * Immutable default implementation of the {@link HttpDestination} interface.
@@ -66,6 +67,13 @@ public final class DefaultHttpDestination implements HttpDestination
     @Getter( AccessLevel.PACKAGE )
     private final ImmutableList<DestinationHeaderProvider> customHeaderProviders;
 
+    /**
+     * Lazily initialized and cached header providers loaded via FacadeLocator. This ensures the same instances are
+     * shared across all DefaultHttpDestination instances. Uses volatile to ensure visibility of changes across threads.
+     */
+    @Nullable
+    private static volatile ImmutableList<DestinationHeaderProvider> cachedHeaderProvidersFromClassLoading;
+
     @Nonnull
     private final ImmutableList<DestinationHeaderProvider> headerProvidersFromClassLoading;
 
@@ -114,10 +122,7 @@ private DefaultHttpDestination(
         this.customHeaders =
             customHeaders != null ? ImmutableList.<Header> builder().addAll(customHeaders).build() : ImmutableList.of();
 
-        final Collection<DestinationHeaderProvider> headerProvidersFromClassLoading =
-            FacadeLocator.getFacades(DestinationHeaderProvider.class);
-        this.headerProvidersFromClassLoading =
-            ImmutableList.<DestinationHeaderProvider> builder().addAll(headerProvidersFromClassLoading).build();
+        this.headerProvidersFromClassLoading = getCachedHeaderProvidersFromClassLoading();
 
         this.customHeaderProviders =
             customHeaderProviders != null
@@ -144,6 +149,32 @@ private DefaultHttpDestination(
                 .build();
     }
 
+    /**
+     * Lazily initializes and returns the cached header providers from class loading. Uses double-checked locking to
+     * ensure thread-safe lazy initialization while minimizing synchronization overhead.
+     *
+     * @return The immutable list of header providers loaded via FacadeLocator.
+     */
+    @Nonnull
+    private static ImmutableList<DestinationHeaderProvider> getCachedHeaderProvidersFromClassLoading()
+    {
+        ImmutableList<DestinationHeaderProvider> cached = cachedHeaderProvidersFromClassLoading;
+        if( cached == null ) {
+            synchronized( DefaultHttpDestination.class ) {
+                cached = cachedHeaderProvidersFromClassLoading;
+                if( cached == null ) {
+                    cached =
+                        ImmutableList
+                            .<DestinationHeaderProvider> builder()
+                            .addAll(FacadeLocator.getFacades(DestinationHeaderProvider.class))
+                            .build();
+                    cachedHeaderProvidersFromClassLoading = cached;
+                }
+            }
+        }
+        return cached;
+    }
+
     /**
      * Verifies that the given "generic" destination might be decorated into a {@code DefaultHttpDestination}.
      *
@@ -511,8 +542,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 +566,43 @@ 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));
+
+        for( int i = 0; i < customHeaderProviders.size(); i++ ) {
+            builder.append(customHeaderProviders.get(i), that.customHeaderProviders.get(i));
+        }
+        for( int i = 0; i < headerProvidersFromClassLoading.size(); i++ ) {
+            builder.append(headerProvidersFromClassLoading.get(i), that.headerProvidersFromClassLoading.get(i));
+        }
+        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,11 +1,17 @@
 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;
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -14,12 +20,12 @@
 
 import org.apache.http.client.HttpClient;
 import org.apache.http.client.methods.HttpGet;
-import org.apache.http.client.methods.HttpUriRequest;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.parallel.Isolated;
 
+import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
 import com.sap.cloud.sdk.cloudplatform.cache.CacheManager;
 import com.sap.cloud.sdk.cloudplatform.connectivity.exception.HttpClientInstantiationException;
 import com.sap.cloud.sdk.cloudplatform.security.principal.DefaultPrincipal;
@@ -28,9 +34,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
@@ -178,42 +190,6 @@ void testGetClientWithDestinationUsesTenantOptionalForIsolation()
         assertThat(tenantClients).size().isEqualTo(3);
     }
 
-    @Test
-    //This is a known limitation of excluding header providers in the equality check of destinations
-    void testGetClientReturnsSameClientForDestinationsWithOnlyDifferentHeaderProviders()
-    {
-        final Header header1 = new Header("foo", "bar");
-        final Header header2 = new Header("foo1", "bar1");
-
-        final DefaultHttpDestination firstDestination =
-            DefaultHttpDestination
-                .builder("http://some-uri")
-                .headerProviders(( any ) -> Collections.singletonList(header1))
-                .build();
-
-        final DefaultHttpDestination secondDestination =
-            DefaultHttpDestination
-                .fromDestination(firstDestination)
-                .headerProviders(( any ) -> Collections.singletonList(header2))
-                .build();
-
-        final HttpClientWrapper client1 = (HttpClientWrapper) sut.tryGetHttpClient(firstDestination, FACTORY).get();
-        final HttpClientWrapper client2 = (HttpClientWrapper) sut.tryGetHttpClient(secondDestination, FACTORY).get();
-
-        assertThat(client1.getDestination()).isSameAs(firstDestination);
-        assertThat(client2.getDestination()).isSameAs(secondDestination);
-
-        final HttpUriRequest request1 = client1.wrapRequest(new HttpGet());
-        final HttpUriRequest request2 = client2.wrapRequest(new HttpGet());
-
-        // This behavior is to be improved by https://github.com/SAP/cloud-sdk-java-backlog/issues/396
-        assertThat(request1.getAllHeaders()).containsExactly(new HttpClientWrapper.ApacheHttpHeader(header1));
-        assertThat(request2.getAllHeaders())
-            .containsExactly(
-                new HttpClientWrapper.ApacheHttpHeader(header1),
-                new HttpClientWrapper.ApacheHttpHeader(header2));
-    }
-
     @Test
     void testGetClientUsesTenantAndPrincipalRequiredForIsolation()
     {
@@ -381,4 +357,85 @@ void testPrincipalPropagationIsPrincipalIsolated()
             .describedAs("Without a principal http clients should not be cached for user based destinations")
             .isInstanceOf(HttpClientInstantiationException.class);
     }
+
+    @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);
+
+        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(((HttpClientWrapper) client1Again).getDestination()).isSameAs(destination1);
+        assertThat(client1Again).isSameAs(client1);
+
+        // 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 List<Header> getHeaders()
+    {
+        return List.of(new Header("Authorization", "Bearer token" + count++));
+    }
 }

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.");
         }