Expect 100 Continue Hang (#38774)
authorMarie Píchová <11718369+ManickaP@users.noreply.github.com>
Tue, 28 Jul 2020 14:58:43 +0000 (16:58 +0200)
committerGitHub <noreply@github.com>
Tue, 28 Jul 2020 14:58:43 +0000 (16:58 +0200)
Inside sendRequestContentTask recognizes that it's invoked from a timer thread and if request content fails, unblocks SendAsyncCore and eventually propagates the exception from the content to the outside.
Fixes the issue for H2 as well.

Fixes #36717

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs
src/libraries/Common/tests/System/Net/Http/ThrowingContent.cs [moved from src/libraries/System.Net.Http/tests/FunctionalTests/ThrowingContent.cs with 88% similarity]
src/libraries/System.Net.Http.WinHttpHandler/tests/FunctionalTests/System.Net.Http.WinHttpHandler.Functional.Tests.csproj
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs
src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj
src/libraries/System.Net.Http/tests/FunctionalTests/ThrowingContent.netcore.cs [new file with mode: 0644]

index 2aa3c26..687fa61 100644 (file)
@@ -2117,6 +2117,45 @@ namespace System.Net.Http.Functional.Tests
         }
 
         [Fact]
+        public async Task SendAsync_Expect100Continue_RequestBodyFails_ThrowsContentException()
+        {
+            if (IsWinHttpHandler && UseVersion >= HttpVersion20.Value)
+            {
+                return;
+            }
+            if (!TestAsync && UseVersion >= HttpVersion20.Value)
+            {
+                return;
+            }
+
+            var clientFinished = new TaskCompletionSource<bool>();
+
+            await LoopbackServerFactory.CreateClientAndServerAsync(async uri =>
+            {
+                using (HttpClient client = CreateHttpClient())
+                {
+                    HttpRequestMessage initialMessage = new HttpRequestMessage(HttpMethod.Post, uri) { Version = UseVersion };
+                    initialMessage.Content = new ThrowingContent(() => new ThrowingContentException());
+                    initialMessage.Headers.ExpectContinue = true;
+                    Exception exception = await Assert.ThrowsAsync<ThrowingContentException>(() => client.SendAsync(TestAsync, initialMessage));
+
+                    clientFinished.SetResult(true);
+                }
+            }, async server =>
+            {
+                await server.AcceptConnectionAsync(async connection =>
+                {
+                    try
+                    {
+                        await connection.ReadRequestDataAsync(readBody: true);
+                    }
+                    catch { } // Eat errors from client disconnect.
+                    await clientFinished.Task.TimeoutAfter(TimeSpan.FromMinutes(2));
+                });
+            });
+        }
+
+        [Fact]
         public async Task SendAsync_No100ContinueReceived_RequestBodySentEventually()
         {
             if (IsWinHttpHandler && UseVersion >= HttpVersion20.Value)
@@ -10,7 +10,7 @@ using System.Threading.Tasks;
 namespace System.Net.Http.Functional.Tests
 {
     /// <summary>HttpContent that mocks exceptions on serialization.</summary>
-    public class ThrowingContent : HttpContent
+    public partial class ThrowingContent : HttpContent
     {
         private readonly Func<Exception> _exnFactory;
         private readonly int _length;
@@ -32,4 +32,7 @@ namespace System.Net.Http.Functional.Tests
             return true;
         }
     }
+
+    public class ThrowingContentException : Exception
+    { }
 }
index b1dbc73..1d0dcec 100644 (file)
              Link="Common\System\Net\Http\SchSendAuxRecordHttpTest.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\SyncBlockingContent.cs"
              Link="Common\System\Net\Http\SyncBlockingContent.cs" />
+    <Compile Include="$(CommonTestPath)System\Net\Http\ThrowingContent.cs"
+             Link="Common\System\Net\Http\ThrowingContent.cs" />
     <Compile Include="$(CommonTestPath)System\Threading\Tasks\TaskTimeoutExtensions.cs"
              Link="Common\System\Threading\Tasks\TaskTimeoutExtensions.cs" />
     <Compile Include="$(CommonTestPath)System\Threading\TrackingSynchronizationContext.cs"
index 83f7953..b21fd4a 100644 (file)
@@ -1747,7 +1747,8 @@ namespace System.Net.Http
                 if (requestBodyTask.IsCompleted ||
                     duplex == false ||
                     await Task.WhenAny(requestBodyTask, responseHeadersTask).ConfigureAwait(false) == requestBodyTask ||
-                    requestBodyTask.IsCompleted)
+                    requestBodyTask.IsCompleted ||
+                    http2Stream.SendRequestFinished)
                 {
                     // The sending of the request body completed before receiving all of the request headers (or we're
                     // ok waiting for the request body even if it hasn't completed, e.g. because we're not doing duplex).
index f9cee65..5ac829b 100644 (file)
@@ -145,6 +145,8 @@ namespace System.Net.Http
 
             public int StreamId { get; private set; }
 
+            public bool SendRequestFinished => _requestCompletionState != StreamCompletionState.InProgress;
+
             public HttpResponseMessage GetAndClearResponse()
             {
                 // Once SendAsync completes, the Http2Stream should no longer hold onto the response message.
index 4a9485a..780212d 100644 (file)
@@ -692,6 +692,21 @@ namespace System.Net.Http
                 // hook up a continuation that will log it.
                 if (sendRequestContentTask != null && !sendRequestContentTask.IsCompletedSuccessfully)
                 {
+                    // In case the connection is disposed, it's most probable that
+                    // expect100Continue timer expired and request content sending failed.
+                    // We're awaiting the task to propagate the exception in this case.
+                    if (Volatile.Read(ref _disposed) == 1)
+                    {
+                        if (async)
+                        {
+                            await sendRequestContentTask.ConfigureAwait(false);
+                        }
+                        else
+                        {
+                            // No way around it here if we want to get the exception from the task.
+                            sendRequestContentTask.GetAwaiter().GetResult();
+                        }
+                    }
                     LogExceptions(sendRequestContentTask);
                 }
 
@@ -793,7 +808,8 @@ namespace System.Net.Http
         }
 
         private async Task SendRequestContentWithExpect100ContinueAsync(
-            HttpRequestMessage request, Task<bool> allowExpect100ToContinueTask, HttpContentWriteStream stream, Timer expect100Timer, bool async, CancellationToken cancellationToken)
+            HttpRequestMessage request, Task<bool> allowExpect100ToContinueTask,
+            HttpContentWriteStream stream, Timer expect100Timer, bool async, CancellationToken cancellationToken)
         {
             // Wait until we receive a trigger notification that it's ok to continue sending content.
             // This will come either when the timer fires or when we receive a response status line from the server.
@@ -806,7 +822,17 @@ namespace System.Net.Http
             if (sendRequestContent)
             {
                 if (NetEventSource.Log.IsEnabled()) Trace($"Sending request content for Expect: 100-continue.");
-                await SendRequestContentAsync(request, stream, async, cancellationToken).ConfigureAwait(false);
+                try
+                {
+                    await SendRequestContentAsync(request, stream, async, cancellationToken).ConfigureAwait(false);
+                }
+                catch
+                {
+                    // Tear down the connection if called from the timer thread because caller's thread will wait for server status line indefinitely
+                    // or till HttpClient.Timeout tear the connection itself.
+                    Dispose();
+                    throw;
+                }
             }
             else
             {
index dc5bc00..2e00a89 100644 (file)
     <Compile Include="$(CommonTestPath)System\Net\Http\HttpClientHandlerTest.Proxy.cs"
              Link="Common\System\Net\Http\HttpClientHandlerTest.Proxy.cs" />
     <Compile Include="HttpClientHandlerTest.Http3.cs" />
-
     <Compile Include="HttpClientHandlerTest.ResponseDrain.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\HttpClientHandlerTest.ServerCertificates.cs"
              Link="Common\System\Net\Http\HttpClientHandlerTest.ServerCertificates.cs" />
              Link="Common\System\Net\Http\SyncBlockingContent.cs" />
     <Compile Include="$(CommonTestPath)System\Net\Http\DefaultCredentialsTest.cs"
              Link="Common\System\Net\Http\DefaultCredentialsTest.cs" />
-    <Compile Include="ThrowingContent.cs" />
+    <Compile Include="$(CommonTestPath)System\Net\Http\ThrowingContent.cs"
+             Link="Common\System\Net\Http\ThrowingContent.cs" />
+    <Compile Include="ThrowingContent.netcore.cs" />
     <Compile Include="Watchdog.cs" />
   </ItemGroup>
   <!-- Windows specific files -->
diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/ThrowingContent.netcore.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/ThrowingContent.netcore.cs
new file mode 100644 (file)
index 0000000..8f4fd47
--- /dev/null
@@ -0,0 +1,17 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.IO;
+using System.Threading;
+using System.Threading.Tasks;
+
+namespace System.Net.Http.Functional.Tests
+{
+    public partial class ThrowingContent : HttpContent
+    {
+        protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken)
+        {
+            throw _exnFactory();
+        }
+    }
+}