Add connection caching using System properties #39229

samvaity · 2024-03-14T23:17:49Z

Perf Results:

Simple Http GET

Request Throughput

Simple Http Download

Request Throughput

Simple Http PATCH

Request Throughput

05/1 perf update:

Simple Http GET:

Okhttp and Default Client have comparable results.
Memory usage is better with DefaultCLient

Simple Http PATCH

Request Throughput

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnection.java

alzimmermsft · 2024-04-22T16:33:34Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnection.java

+        if (socketInputStream != null) {
+            socketInputStream.close();
+        }
+        if (socketOutputStream != null) {
+            socketOutputStream.close();
+        }


Closing the socket will also close the streams

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnection.java

sdk/client-core/http-stress/src/main/java/io/clientcore/http/stress/App.java

sdk/client-core/core/src/test/java/io/clientcore/core/models/SocketConnectionCacheTest.java

alzimmermsft · 2024-05-03T18:47:43Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnectionCache.java

+     * @param readTimeout read timeout for the connection
+     * @return the instance of the SocketConnectionCache if it exists, else create a new one
+     */
+    public static synchronized SocketConnectionCache getInstance(boolean connectionKeepAlive, int maximumConnections,


Few thoughts on the design

If connectionKeepAlive is false, why would we want to create or get this? I'd say let's simplify some stuff where whatever keep alive and maximum connections is as the start of the HttpClient we use those for the lifetime and only need to call this once as a static singleton.

Why do we take readTimeout? This is something that may change HttpClient to HttpClient and is only used when reading from the network, nothing to do with the connection pool itself. Unless this was supposed to be a connection idle timeout.

readTimeout is unrelated from the connection pooling work actually, noticed we were missing doing this in the patch so went ahead and added it.

Yeah, adding the read timeout is good, just not sure this is the right place for it.

alzimmermsft · 2024-05-03T18:48:44Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnectionCache.java

+    public SocketConnection get(SocketConnectionProperties socketConnectionProperties) throws IOException {
+        SocketConnection connection = null;
+        // try to get a connection from the cache
+        synchronized (CONNECTION_POOL) {


nit: we shouldn't be using synchronized, this reduces scalability when using virtual threads. We should look into using things like ReentrantLock or Semaphore for concurrency control.

alzimmermsft · 2024-05-03T18:50:09Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnectionCache.java

+    private final int readTimeout;
+    private final boolean keepConnectionAlive;
+    private final int maxConnections;
+    private long readPos = 0;


What's the purpose of this? Shouldn't this be something specific to an individual connection and not the cache itself

alzimmermsft · 2024-05-03T18:53:23Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnection.java

+    public static final class SocketConnectionProperties {
+        private final URL requestUrl;
+        private final String host;
+        private final String port;


Why do we convert the port to a String?

alzimmermsft · 2024-05-03T19:01:48Z

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

+        private static int readStatusCode(InputStream inputStream) throws IOException {
+            ByteArrayOutputStream byteOutputStream = new ByteArrayOutputStream();
+            int b;
+            while ((b = inputStream.read()) != -1 && b != '\n') {


One thing we may need to look at in the future for performance is reading in chunks, reading byte-by-byte off the network will be costly.

Since we are dealing with persistent connections and inputstreams, we should be better off reading byte-by-byte rather than buffering readers or readLines I was thinking.
What could be the alternative otherwise?

I believe the Socket stream will return -1 even if the connection can be reused later.

alzimmermsft · 2024-05-03T19:04:24Z

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

+            }
+            String[] parts = statusLine.split(" ");
+            if (parts.length < 2) {
+                throw new IllegalStateException("Unexpected response from server. Status : " + statusLine);


When exceptions are thrown during processing, are we closing the connection?

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

alzimmermsft · 2024-05-03T19:06:44Z

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

+            throws IOException {
+            int contentLength;
+            if (contentLengthHeader == null || contentLengthHeader.getValue() == null) {
+                contentLength = -1;


Any reason we don't just return null here?

alzimmermsft · 2024-05-03T19:07:38Z

sdk/client-core/core/src/main/java/io/clientcore/core/http/client/DefaultHttpClient.java

+                int bytesRead;
+                int totalBytesRead = 0;
+
+                while (totalBytesRead < contentLength


This could be a problem if there is an invalid response where the amount of data differs from the Content-Length value. We probably should just read all the data and then if the read amount differs from what the Content-Length header specified throw an exception.

Should we rather throw when the "amount of data differs from the Content-Length value" instead?

Yeah, but I'd do it after completing the reading

alzimmermsft · 2024-05-03T19:09:56Z

sdk/client-core/core/src/main/java/io/clientcore/core/models/SocketConnection.java

+            this.requestUrl = requestUrl;
+            this.host = host;
+            this.port = port;
+            this.sslSocketFactory = sslSocketFactory;


Should include the SSLSocketFactory in the equivalence check and hash code.

alzimmermsft

Let's get this merged in so we can begin iterating on focus areas.

samvaity force-pushed the connectio-pool-2 branch from 686d9bc to d3b803a Compare March 26, 2024 01:12

samvaity linked an issue Apr 2, 2024 that may be closed by this pull request

Add connection caching for socket-based PATCH requests #38242

Closed

samvaity force-pushed the connectio-pool-2 branch from d3b803a to 2c94e73 Compare April 2, 2024 21:04

samvaity self-assigned this Apr 3, 2024

samvaity force-pushed the connectio-pool-2 branch 7 times, most recently from d79bc5e to 8365f95 Compare April 22, 2024 14:43

samvaity marked this pull request as ready for review April 22, 2024 14:48

samvaity requested review from vcolin7, JonathanGiles, srnagar and alzimmermsft as code owners April 22, 2024 14:48

alzimmermsft reviewed Apr 22, 2024

View reviewed changes

sdk/client-core/core/src/test/java/io/clientcore/core/models/SocketConnectionCacheTest.java Outdated Show resolved Hide resolved

samvaity force-pushed the connectio-pool-2 branch from a0c314d to 7daed8e Compare April 25, 2024 17:51

samvaity mentioned this pull request Apr 25, 2024

Add connection caching for HttpUrlConnectionClient #39199

Closed

samvaity force-pushed the connectio-pool-2 branch 2 times, most recently from 9086dd5 to 0938cec Compare May 1, 2024 22:04

samvaity added 8 commits May 3, 2024 11:34

update to use system properties

72a9255

update socket connection for patch

116a906

update for keep-alive + response handling

175b801

cleanup

4ff05ac

update for stream closing

07e3200

add patch stress test changes

8a3a425

add unit tests

4b1ef20

checktyle updates

f14930b

samvaity added 6 commits May 3, 2024 11:34

patch stress test updates

1511ccb

add dynamic port binding

8d5a3c1

update to read instead of bufferedreader

4db4a9f

update for skip

2e525c3

fix reading headers

936eaa7

update URL

34fc4ff

samvaity force-pushed the connectio-pool-2 branch from 17dec21 to 34fc4ff Compare May 3, 2024 18:34

alzimmermsft reviewed May 3, 2024

View reviewed changes

samvaity force-pushed the connectio-pool-2 branch from f580f7f to 6278744 Compare May 6, 2024 17:45

simplify + singleon initialize cache instance

05470a7

samvaity force-pushed the connectio-pool-2 branch from 6278744 to 05470a7 Compare May 6, 2024 21:25

checkstyle updates

6bc0c96

alzimmermsft approved these changes May 8, 2024

View reviewed changes

samvaity merged commit 9319fa1 into Azure:feature/generic-sdk-core-2 May 8, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection caching using System properties #39229

Add connection caching using System properties #39229

samvaity commented Mar 14, 2024 •

edited

alzimmermsft Apr 22, 2024

alzimmermsft May 3, 2024

samvaity May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

samvaity May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

samvaity May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft May 3, 2024

alzimmermsft left a comment

Add connection caching using System properties #39229

Add connection caching using System properties #39229

Conversation

samvaity commented Mar 14, 2024 • edited

Perf Results:

Simple Http GET

Request Throughput

Simple Http Download

Request Throughput

Simple Http PATCH

Request Throughput

Simple Http GET:

Simple Http PATCH

Request Throughput

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft left a comment

Choose a reason for hiding this comment

samvaity commented Mar 14, 2024 •

edited