Performance issue in scim2-sdk-client

[gclayburg]

I've noticed that the scim2-sdk-client does not completely consume result entity data from client operations. The effect is that the underlying TCP stream is automatically closed after each operation. A new TCP stream must be re-negotiated for a subsequent request. This is all automatic under Jersey, but it causes a delay that is especially noticeable when any of these things are true:

• there are many client requests
• TLS is used to secure the connection
• there is significant latency to the target server.

This can be reproduced with this client code:

        ScimService scimService = new ScimService(target);
        for (int i = 0; i < 100; i++) {
            UserResource newUser = new UserResource().setUserName("createLoopUser" + i);
            UserResource createdUser = scimService.createRequest("Users", newUser).
                    contentType(MediaType.APPLICATION_JSON).invoke();
            Assert.assertNotNull(createdUser);
        }

This code will complete without errors, but you can observe the unnecessary network activity by examining a wireshark trace of the TCP/IP network communications.

[kqarryzada]

Hi @gclayburg,

Thanks for submitting this and for all of the details you've provided. I've looked at the change in the PR (with calls to Response.bufferEntity()) to get some context on what you're suggesting. However, it doesn't appear to me that it actually addresses the root problem as you've described.

The bufferEntity() method appears to stall thread processing, fully read and cache the network response in memory, and then continue the thread when the end of the stream has been reached. While it's true that this call itself does not close the TCP connection, I don't believe that adding this would change anything. This is because the SCIM SDK always closes the Response object automatically due to the try-with-resources statements, as shown below. In older releases (ones that line up with the time that you had opened this ticket), there was still a dedicated finally block that closed it manually, but the behavior is the same regardless:

response.bufferEntity();
try (InputStream inputStream = response.readEntity(...))
{
    ...
}

As far as I can tell, every connection is always going to be closed for each API response regardless of whether we include bufferEntity(). Furthermore, most of our calls that do not read the data into an InputStream should already be blocking to wait for the full network response as far as I can tell.

 

I think the real answer to address the penalty of opening and closing connections en masse, or in unstable network conditions, is connection pooling. With connections being established and then held and reused, you'll skip the repeated TCP and TLS round trips for most requests. This is beneficial in any of the cases you mentioned:

• Large volumes of requests means you're establishing and maintaining several hundred TCP connections as opposed to directly opening and closing thousands, millions, or more.
• TLS negotiation occurs less often (and should always be used when transmitting sensitive SCIM identity data regardless).
• Slow network connections also gain some benefit from avoiding costly round trips when there is extra latency to transmit simple ACK messages.

Unfortunately, the SCIM SDK client is a Jakarta-RS interface and does not contain an existing mechanism that could easily provide connection pooling for free. However, applications leveraging the SCIM SDK client often rely on libraries like Jersey, and these do have the capability to provide connection pooling. Here's an example showcasing the required configuration in the Jersey-based EndpointTestCase. This leans on the Apache HTTP Client to provide the connection pooling implementation:

diff --git a/scim2-sdk-server/src/test/java/com/unboundid/scim2/server/EndpointTestCase.java b/scim2-sdk-server/src/test/java/com/unboundid/scim2/server/EndpointTestCase.java
index 29b67560..0569f916 100644
--- a/scim2-sdk-server/src/test/java/com/unboundid/scim2/server/EndpointTestCase.java
+++ b/scim2-sdk-server/src/test/java/com/unboundid/scim2/server/EndpointTestCase.java
@@ -79,6 +79,8 @@ import com.unboundid.scim2.server.resources.ResourceTypesEndpoint;
 import com.unboundid.scim2.server.resources.SchemasEndpoint;
 import com.unboundid.scim2.server.utils.ResourceTypeDefinition;
 import com.unboundid.scim2.server.utils.ServerUtils;
+import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder;
+import org.glassfish.jersey.apache5.connector.Apache5ClientProperties;
 import org.glassfish.jersey.apache5.connector.Apache5ConnectorProvider;
 import org.glassfish.jersey.client.ClientConfig;
 import org.glassfish.jersey.client.ClientProperties;
@@ -98,6 +100,7 @@ import jakarta.ws.rs.core.MediaType;
 import jakarta.ws.rs.core.Response;
 import jakarta.ws.rs.core.UriBuilder;
 import java.net.URI;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -167,6 +170,16 @@ public class EndpointTestCase extends JerseyTestNg.ContainerPerClassTest
   @Override
   protected void configureClient(final ClientConfig config)
   {
+    // Set configuration parameters for connections within a pool. This
+    // connection pool will be closed when the Client is closed.
+    var connectionManager = PoolingHttpClientConnectionManagerBuilder.create()
+        .setMaxConnTotal(200)
+        .setMaxConnPerRoute(100)
+        .build();
+    config.property(Apache5ClientProperties.CONNECTION_MANAGER, connectionManager);
+
+    config.property(ClientProperties.CONNECT_TIMEOUT, Duration.ofSeconds(10).toMillis());
+
     config.connectorProvider(new Apache5ConnectorProvider());
   }

Hopefully this alternate approach is helpful, since it feels like the more appropriate solution for managing connection overhead. Since the proposed change has drawbacks and it should be feasible to configure connection pooling, I don't think there any changes to make within the SCIM SDK. Thank you again for opening this for consideration. I'll close this, but let me know if you have questions.