Fix HttpClient object leak described in #5948 (no socket exhaustion) by bgavrilMS · Pull Request #5949 · AzureAD/microsoft-authentication-library-for-dotnet

bgavrilMS · 2026-04-24T13:55:05Z

Fix a leak of HttpCLient objects from improper use of ConcurrentDictionary

Copilot

Pull request overview

Fixes an unnecessary HttpClient allocation pattern in SimpleHttpClientFactory caused by eager evaluation of ConcurrentDictionary.GetOrAdd value arguments, and adds a regression test to ensure repeated lookups do not create throwaway clients.

Changes:

Update ConcurrentDictionary.GetOrAdd usage to use value-factory delegates (lazy creation) for both non-mTLS and mTLS clients.
Add unit test coverage to assert CreateHttpClient is only invoked once for repeated default-key lookups.
Add test instrumentation (HttpClientCreationCount + reset helper) to support the new regression test.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
tests/Microsoft.Identity.Test.Unit/CoreTests/HttpTests/HttpClientFactoryTests.cs	Adds regression test validating no repeated `HttpClient` creation for repeated `GetHttpClient()` calls.
src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs	Fixes eager `GetOrAdd` evaluation by switching to delegates; adds test-only instrumentation and reset helper.

Copilot · 2026-04-24T13:59:04Z

+        internal static void ResetForTest()
+        {
+            s_httpClientPool.Clear();
+            HttpClientCreationCount = 0;
+        }


ResetForTest clears the shared HttpClient pool without disposing the existing HttpClient instances. Since this helper is intended for tests starting from a clean slate, consider disposing the pooled clients before clearing to avoid leaving handlers/resources alive for the remainder of the test run.

@copilot apply changes based on this feedback

Applied in commit 30bec75. ResetStaticStateForTest now iterates over the pooled values and calls Dispose() on each HttpClient before clearing the dictionary.

gladjohn · 2026-04-24T14:20:47Z

Nit: PR title references #5984 but should be #5948.

…entFactoryTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…-only property Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/a79a3efc-2df8-40a0-af39-3ec2083e19ac Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

…setForTest Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/0be221f9-e45c-4835-b968-585fb1aa0082 Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

Copilot · 2026-04-24T18:25:51Z


+        // referenced in unit tests
+        internal static void ResetStaticStateForTest()
+        {
+            foreach (var client in s_httpClientPool.Values)
+            {
+                client.Dispose();


These members are explicitly test-only hooks but live in the production assembly/class. To reduce long-term maintenance risk and avoid accidental non-test usage, consider isolating them behind conditional compilation (e.g., only included in test builds) or moving them into a test-only partial class/file compiled only for tests.

Suggested change

// referenced in unit tests

internal static void ResetStaticStateForTest()

{

foreach (var client in s_httpClientPool.Values)

{

client.Dispose();

#if DEBUG

// referenced in unit tests

internal static void ResetForTest()

{

s_httpClientPool.Clear();

s_httpClientCreationCount = 0;

}

#endif

Robbie-Microsoft · 2026-04-24T19:36:11Z

The Service Fabric overload still has the unfixed eager GetOrAdd pattern:

// GetHttpClient(Func<...> validateServerCert) — line ~111
return s_httpClientPool.GetOrAdd(key, new HttpClient(handler));

But there's a deeper problem here: the key is handler.GetHashCode().ToString(). Since GetHashCode() on a reference type returns the object's identity hash, and a new HttpClientHandler is created on every call, the key is always unique — the pool never actually hits. Every call adds a new HttpClient to the pool permanently (until the 1000-entry eviction), which is worse than the bug being fixed.

The comment above it even acknowledges this: "The server cert rotates so we need a new HttpClient for each call." If that's the intent, the GetOrAdd should be removed entirely and the method should just return new HttpClient(handler) directly, skipping the pool.

Robbie-Microsoft · 2026-04-24T19:36:21Z

+        }
+
    }
 }


The regression test only covers GetHttpClient() (the "non_mtls" key). GetHttpClient(X509Certificate2) received the same factory-delegate fix but has no corresponding test. Consider adding a parallel test calling GetHttpClient(cert) multiple times with the same cert and asserting the HttpClientCreationCount only increments once.

Added in commit f212029. TestGetHttpClientWithMtlsCert_DoesNotLeakHttpClients calls GetHttpClient(cert) three times with the same certificate and asserts HttpClientCreationCount == 1. Also added Interlocked.Increment to CreateMtlsHttpClient so the counter tracks mTLS client creation too.

…mpleHttpClientFactory.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-27T17:58:26Z

+                client.Dispose();
+            }
+            s_httpClientPool.Clear();
+            s_httpClientCreationCount = 0;


ResetStaticStateForTest contains leftover/duplicated code after the lock block (client.Dispose();, extra braces, and an extra s_httpClientPool.Clear() / s_httpClientCreationCount = 0). As written it will not compile (client is undefined) and the braces are unbalanced. Please remove the stray lines and keep the reset logic inside the lock (disposing pooled clients, clearing the pool, and resetting the counter).

Suggested change

client.Dispose();

}

s_httpClientPool.Clear();

s_httpClientCreationCount = 0;

Copilot · 2026-04-27T17:58:26Z

+                foreach (KeyValuePair<string, HttpClient> pooledClient in s_httpClientPool)
+                {
+                    pooledClient.Value?.Dispose();
+                }
+
+                s_httpClientPool.Clear();
+                Interlocked.Exchange(ref s_httpClientCreationCount, 0);
+            }
+                client.Dispose();
+            }
+            s_httpClientPool.Clear();
+            s_httpClientCreationCount = 0;


ResetStaticStateForTest uses KeyValuePair<string, HttpClient> but this file doesn’t import System.Collections.Generic, and the main library project doesn’t enable implicit usings. This should be a compile error; either add the missing using or iterate s_httpClientPool.Values (or use var) to avoid the extra type reference.

Suggested change

foreach (KeyValuePair<string, HttpClient> pooledClient in s_httpClientPool)

{

pooledClient.Value?.Dispose();

}

s_httpClientPool.Clear();

Interlocked.Exchange(ref s_httpClientCreationCount, 0);

}

client.Dispose();

}

s_httpClientPool.Clear();

s_httpClientCreationCount = 0;

foreach (HttpClient pooledClient in s_httpClientPool.Values)

{

pooledClient?.Dispose();

}

s_httpClientPool.Clear();

Interlocked.Exchange(ref s_httpClientCreationCount, 0);

}

…LS cert test; fix ResetStaticStateForTest Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/680737a8-8460-40bd-8a4a-9e32d96560f0 Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

Copilot · 2026-04-27T18:01:34Z

The Service Fabric overload still has the unfixed eager GetOrAdd pattern:

...

Fixed in commit f212029. Since every call creates a new HttpClientHandler with a unique identity hash, the pool would never actually hit and every entry would be permanently added. Per the comment in the code ("The server cert rotates so we need a new HttpClient for each call"), the GetOrAdd was removed entirely — the method now just returns new HttpClient(handler) directly.

…truction under contention Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/4cfee597-417f-4608-acad-96814445f7a1 Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-28T07:08:51Z

@@ -101,14 +130,13 @@
 #if NET471_OR_GREATER || NETSTANDARD || NET
            var handler = new HttpClientHandler
            {
                ServerCertificateCustomValidationCallback = (message, cert, chain, sslPolicyErrors) =>
                {
                    return validateServerCert(message, cert, chain, sslPolicyErrors);
                }
            };

-            string key = handler.GetHashCode().ToString();
-            return s_httpClientPool.GetOrAdd(key, new HttpClient(handler));
+            return new HttpClient(handler);


GetHttpClient(validateServerCert) now always constructs a new HttpClient per call. This conflicts with IMsalSFHttpClientFactory guidance (“Do not create a new HttpClient for each call … this leads to socket exhaustion”) and can cause connection/socket exhaustion in Service Fabric MI scenarios. Consider caching/reusing an HttpClient per stable callback key (similar to the non-mTLS/mTLS pools) so repeated calls with the same delegate reuse the same client/handler.

The expensive stuff is not the creation of a new HttpClient, it's the creation of the handler? And that code hasn't changed.

Service Fabric scenario should not be HTTP heavy. What is your suggestion?

Fixed in commit af9d6ca. GetHttpClient(validateServerCert) now caches HttpClient instances in s_httpClientPool keyed by the delegate's method handle + target object identity (RuntimeHelpers.GetHashCode). This is stable even when a new delegate wrapper is created per call (the common Service Fabric case — return ValidateServerCertificateCallback on the same source instance). Also added HttpClientConfig.ConfigureRequestHeadersAndSize for consistency with the other overloads, and updated the tests to assert the new caching semantics.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-28T12:01:34Z

 #if NET471_OR_GREATER || NETSTANDARD || NET
            var handler = new HttpClientHandler
            {
                ServerCertificateCustomValidationCallback = (message, cert, chain, sslPolicyErrors) =>
                {
                    return validateServerCert(message, cert, chain, sslPolicyErrors);
                }
            };

-            string key = handler.GetHashCode().ToString();
-            return s_httpClientPool.GetOrAdd(key, new HttpClient(handler));
+            return new HttpClient(handler);
 #else


GetHttpClient(validateServerCert) returns a new HttpClient on every call and the caller (HttpManager) does not dispose it, which can lead to socket exhaustion under the Service Fabric managed identity flow. Consider caching this client (e.g., in s_httpClientPool keyed by the callback/delegate identity) or updating the call chain to deterministically dispose per-call clients; also apply HttpClientConfig.ConfigureRequestHeadersAndSize for consistency with the other factory methods.

Copilot · 2026-04-28T12:01:35Z

+        internal static void ResetStaticStateForTest()
+        {
+            lock (s_cacheLock)
+            {
+                foreach (Lazy<HttpClient> lazy in s_httpClientPool.Values)
+                {
+                    if (lazy.IsValueCreated)
+                        lazy.Value?.Dispose();
+                }
+
+                s_httpClientPool.Clear();
+                Interlocked.Exchange(ref s_httpClientCreationCount, 0);
+            }


When the pool reaches the threshold, CheckAndManageCache clears s_httpClientPool without disposing the created HttpClient instances. Since you already have disposal logic here for tests, consider extracting/reusing it so cache eviction also disposes any Lazy<HttpClient> values that were created, otherwise handlers/sockets can linger until GC/finalization.

… avoid socket exhaustion Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/97bbfa6a-513f-4c41-a80c-f8ee33f6e31f Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

Copilot · 2026-04-30T21:25:35Z

The Service Fabric overload still has the unfixed eager GetOrAdd pattern:

...

The broken GetOrAdd pattern was removed in commit f212029 (the pool entry was also removed). The follow-up commit af9d6ca then added proper caching using the delegate's method handle + target identity as the key, so repeated calls with the same callback now reuse the cached HttpClient instead of creating a new one per call.

Nit: PR title references #5984 but should be #5948.

The PR title can be edited directly in the GitHub PR interface — I'm not able to update it from code changes. The correct issue number is #5948 as noted.

gladjohn

clean fix for the eager GetOrAdd leak, Lazy with ExecutionAndPublication is the right call

bgavrilMS · 2026-05-01T09:45:56Z

+            // This correctly caches clients when the same method on the same object instance
+            // is used as the callback (the common Service Fabric case), while still creating
+            // distinct clients for genuinely different callbacks.
+            string key = "sf_" +


@copilot - this is too complicated. Please revert everything related to Service Fabric

Reverted in commit 9e7d2e6. The Service Fabric overload is back to returning new HttpClient(handler) directly with no caching or delegate-identity key logic.

…pClient per call Agent-Logs-Url: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/sessions/55a48d86-6457-42b4-9fd6-d21d0c5683d2 Co-authored-by: bgavrilMS <12273384+bgavrilMS@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 24, 2026 13:55

bgavrilMS requested a review from a team as a code owner April 24, 2026 13:55

bgavrilMS requested a review from gladjohn April 24, 2026 13:55

Copilot started reviewing on behalf of bgavrilMS April 24, 2026 13:55 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

gladjohn approved these changes Apr 24, 2026

View reviewed changes

Copilot started work on behalf of bgavrilMS April 24, 2026 18:17 View session

Copilot AI review requested due to automatic review settings April 24, 2026 18:18

bgavrilMS changed the title ~~Fix HttpClient object leak described in #5984 (no socket exhaustion)~~ Fix HttpClient object leak described in #5948 (no socket exhaustion) Apr 24, 2026

bgavrilMS and others added 2 commits April 24, 2026 19:18

Fix HttpClient object leak described in #5984 (no socket exhaustion)

e7c721a

Update tests/Microsoft.Identity.Test.Unit/CoreTests/HttpTests/HttpCli…

e5e0676

…entFactoryTests.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

bgavrilMS force-pushed the bogavril/5948 branch from 41e5b4c to e5e0676 Compare April 24, 2026 18:18

Copilot finished work on behalf of bgavrilMS April 24, 2026 18:22

Copilot started work on behalf of bgavrilMS April 24, 2026 18:22 View session

Copilot finished work on behalf of bgavrilMS April 24, 2026 18:25

Copilot AI reviewed Apr 24, 2026

View reviewed changes

Copilot started reviewing on behalf of bgavrilMS April 24, 2026 18:38 View session

Robbie-Microsoft reviewed Apr 24, 2026

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs Outdated

Robbie-Microsoft reviewed Apr 24, 2026

View reviewed changes

Comment thread src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs Outdated

Robbie-Microsoft reviewed Apr 24, 2026

View reviewed changes

Update src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/Si…

d3b8667

…mpleHttpClientFactory.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings April 27, 2026 17:55

Copilot started work on behalf of bgavrilMS April 27, 2026 17:57 View session

Copilot AI reviewed Apr 27, 2026

View reviewed changes

Copilot finished work on behalf of bgavrilMS April 27, 2026 18:02

Copilot started work on behalf of bgavrilMS April 28, 2026 07:01 View session

Merge branch 'main' into bogavril/5948

02232cb

Copilot AI review requested due to automatic review settings April 28, 2026 07:02

bgavrilMS enabled auto-merge (squash) April 28, 2026 07:02

Copilot started reviewing on behalf of bgavrilMS April 28, 2026 07:02 View session

auto-merge was automatically disabled April 28, 2026 07:04
Head branch was pushed to by a user without write access

Copilot finished work on behalf of bgavrilMS April 28, 2026 07:06

Copilot AI reviewed Apr 28, 2026

View reviewed changes

bgavrilMS requested review from Robbie-Microsoft and Copilot April 28, 2026 11:54

Copilot started reviewing on behalf of bgavrilMS April 28, 2026 11:55 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Copilot started work on behalf of bgavrilMS April 30, 2026 21:19 View session

Copilot finished work on behalf of bgavrilMS April 30, 2026 21:26

gladjohn approved these changes Apr 30, 2026

View reviewed changes

gladjohn mentioned this pull request May 1, 2026

[Engineering task] Copilot PR Review: excessive noise from review_on_push and false positives #5967

Open

bgavrilMS commented May 1, 2026

View reviewed changes

Copilot started work on behalf of bgavrilMS May 1, 2026 09:46 View session

Copilot AI review requested due to automatic review settings May 1, 2026 09:48

bgavrilMS review requested due to automatic review settings May 1, 2026 09:48

Copilot finished work on behalf of bgavrilMS May 1, 2026 09:49

+                      }
                   }
               }

Conversation

bgavrilMS commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gladjohn commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Robbie-Microsoft commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Robbie-Microsoft Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Apr 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

bgavrilMS Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Robbie-Microsoft commented Apr 24, 2026 •

edited

Loading

Copilot AI commented Apr 30, 2026 •

edited

Loading