From bf7cdc5825f94cf67d69ab5f0ea0c2dc9490861b Mon Sep 17 00:00:00 2001
From: Miha Zupan <mihazupan.zupan1@gmail.com>
Date: Tue, 19 Jul 2022 10:42:31 -0700
Subject: [PATCH] Avoid adding an HttpClient.Timeout message to ConnectTimeout
 exceptions (#72274)

* Avoid adding an HttpClient.Timeout message to ConnectTimeout exceptions

* Avoid wrapping unknown OCEs as well

* Update comment

Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>

Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>
---
 .../src/System/Net/Http/HttpClient.cs         | 13 ++++--
 .../tests/FunctionalTests/HttpClientTest.cs   | 42 +++++++++++++++++++
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
index 80f6da1a6de40..83f4ab8db1166 100644
--- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
+++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs
@@ -604,11 +604,18 @@ private void HandleFailure(Exception e, bool telemetryStarted, HttpResponseMessa
                         e = toThrow = new TaskCanceledException(oce.Message, oce.InnerException, cancellationToken);
                     }
                 }
-                else if (!pendingRequestsCts.IsCancellationRequested)
+                else if (cts.IsCancellationRequested && !pendingRequestsCts.IsCancellationRequested)
                 {
-                    // If this exception is for cancellation, but cancellation wasn't requested, either by the caller's token or by the pending requests source,
+                    // If the linked cancellation token source was canceled, but cancellation wasn't requested, either by the caller's token or by the pending requests source,
                     // the only other cause could be a timeout.  Treat it as such.
-                    e = toThrow = new TaskCanceledException(SR.Format(SR.net_http_request_timedout, _timeout.TotalSeconds), new TimeoutException(e.Message, e), oce.CancellationToken);
+
+                    // cancellationToken could have been triggered right after we checked it, but before we checked the cts.
+                    // We must check it again to avoid reporting a timeout when one did not occur.
+                    if (!cancellationToken.IsCancellationRequested)
+                    {
+                        Debug.Assert(_timeout.TotalSeconds > 0);
+                        e = toThrow = new TaskCanceledException(SR.Format(SR.net_http_request_timedout, _timeout.TotalSeconds), new TimeoutException(e.Message, e), oce.CancellationToken);
+                    }
                 }
             }
             else if (e is HttpRequestException && cts.IsCancellationRequested) // if cancellationToken is canceled, cts will also be canceled
diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
index 00589f8f09cbf..b8ef6e6607f42 100644
--- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
+++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs
@@ -798,6 +798,48 @@ public async Task Timeout_SetTo30AndGetResponseQuickly_Success()
             }
         }
 
+        [ConditionalFact(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))]
+        public async Task ConnectTimeout_NotWrappedInMultipleTimeoutExceptions()
+        {
+            using var handler = CreateHttpClientHandler();
+
+            SocketsHttpHandler socketsHttpHandler = GetUnderlyingSocketsHttpHandler(handler);
+
+            socketsHttpHandler.ConnectTimeout = TimeSpan.FromMilliseconds(1);
+            socketsHttpHandler.ConnectCallback = async (context, cancellation) =>
+            {
+                await Task.Delay(-1, cancellation);
+                throw new UnreachableException();
+            };
+
+            using var client = CreateHttpClient(handler);
+            client.Timeout = TimeSpan.FromSeconds(42);
+
+            TaskCanceledException e = await Assert.ThrowsAsync<TaskCanceledException>(() => client.GetAsync(CreateFakeUri()));
+
+            TimeoutException connectTimeoutException = Assert.IsType<TimeoutException>(e.InnerException);
+            Assert.Contains("ConnectTimeout", connectTimeoutException.Message);
+
+            Assert.Null(connectTimeoutException.InnerException);
+            Assert.DoesNotContain("42", e.ToString());
+        }
+
+        [Fact]
+        public async Task UnknownOperationCanceledException_NotWrappedInATimeoutException()
+        {
+            using var client = new HttpClient(new CustomResponseHandler((request, cancellation) =>
+            {
+                throw new OperationCanceledException("Foo");
+            }));
+            client.Timeout = TimeSpan.FromSeconds(42);
+
+            OperationCanceledException e = await Assert.ThrowsAsync<OperationCanceledException>(() => client.GetAsync(CreateFakeUri()));
+
+            Assert.Null(e.InnerException);
+            Assert.Equal("Foo", e.Message);
+            Assert.DoesNotContain("42", e.ToString());
+        }
+
         [Fact]
         public void DefaultProxy_SetNull_Throws()
         {