From 9d4165a8f52658b816dd8fe7d43922899e16d03c Mon Sep 17 00:00:00 2001 From: Mayuki Sawatari Date: Wed, 18 Sep 2024 15:04:15 +0900 Subject: [PATCH 1/2] Fix deadlock in the completion process when the send buffer is full. --- src/YetAnotherHttpHandler/NativeRuntime.cs | 2 +- src/YetAnotherHttpHandler/RequestContext.cs | 33 ++++++++--- src/YetAnotherHttpHandler/ResponseContext.cs | 2 +- .../Http2TestBase.cs | 59 +++++++++++++++++++ .../TestServerForHttp1AndHttp2.cs | 13 ++++ 5 files changed, 99 insertions(+), 10 deletions(-) diff --git a/src/YetAnotherHttpHandler/NativeRuntime.cs b/src/YetAnotherHttpHandler/NativeRuntime.cs index 7d6f1f2..4a37ff8 100644 --- a/src/YetAnotherHttpHandler/NativeRuntime.cs +++ b/src/YetAnotherHttpHandler/NativeRuntime.cs @@ -9,7 +9,7 @@ internal class NativeRuntime public static NativeRuntime Instance { get; } = new NativeRuntime(); private readonly object _lock = new object(); - private int _refCount; + internal /* for unit testing */ int _refCount; private YahaRuntimeSafeHandle? _handle; private NativeRuntime() diff --git a/src/YetAnotherHttpHandler/RequestContext.cs b/src/YetAnotherHttpHandler/RequestContext.cs index c711489..7d82697 100644 --- a/src/YetAnotherHttpHandler/RequestContext.cs +++ b/src/YetAnotherHttpHandler/RequestContext.cs @@ -128,10 +128,30 @@ private async Task RunReadRequestLoopAsync(CancellationToken cancellationToken) } } - private unsafe void WriteBody(Span data) + private void WriteBody(Span data) + { + var retryInterval = 16; //ms + var retryAfter = 0; + + while (!_cancellationTokenSource.IsCancellationRequested && !TryWriteBody(data)) + { + // TODO: + retryAfter += retryInterval; + Thread.Sleep(Math.Min(1000, retryAfter)); + } + } + + private unsafe bool TryWriteBody(Span data) { lock (_handleLock) { + if (!_handle.IsAllocated) + { + // If the handle has been already released, the request is fully completed. + ThrowHelper.ThrowOperationCanceledException(); + return true; // the method never return from here. + } + Debug.Assert(_handle.IsAllocated); if (YahaEventSource.Log.IsEnabled()) YahaEventSource.Log.Trace($"[ReqSeq:{_requestSequence}:State:0x{Handle:X}] Sending the request body: Length={data.Length}"); @@ -146,8 +166,6 @@ private unsafe void WriteBody(Span data) var ctx = _ctxHandle.DangerousGet(); var requestContext = _requestContextHandle.DangerousGet(); - var retryInterval = 16; //ms - var retryAfter = 0; while (!_requestBodyCompleted) { // If the internal buffer is full, yaha_request_write_body returns false. We need to wait until ready to send bytes again. @@ -161,14 +179,11 @@ private unsafe void WriteBody(Span data) { if (YahaEventSource.Log.IsEnabled()) YahaEventSource.Log.Trace($"[ReqSeq:{_requestSequence}:State:0x{Handle:X}] Failed to write the request body. Because the request has been completed."); ThrowHelper.ThrowOperationCanceledException(); - return; + return true; // the method never return from here. } if (YahaEventSource.Log.IsEnabled()) YahaEventSource.Log.Trace($"[ReqSeq:{_requestSequence}:State:0x{Handle:X}] Send buffer is full."); - - // TODO: - retryAfter += retryInterval; - Thread.Sleep(Math.Min(1000, retryAfter)); + return false; } #if DEBUG // Fill memory so that data corruption can be detected on debug build. @@ -188,6 +203,8 @@ private unsafe void WriteBody(Span data) } } } + + return true; } private unsafe void TryCompleteBody(Exception? exception = default) diff --git a/src/YetAnotherHttpHandler/ResponseContext.cs b/src/YetAnotherHttpHandler/ResponseContext.cs index 8e8da0e..bcec29f 100644 --- a/src/YetAnotherHttpHandler/ResponseContext.cs +++ b/src/YetAnotherHttpHandler/ResponseContext.cs @@ -26,7 +26,7 @@ internal ResponseContext(HttpRequestMessage requestMessage, RequestContext reque _requestContext = requestContext; _responseTask = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _cancellationToken = cancellationToken; - _tokenRegistration = cancellationToken.Register((state) => + _tokenRegistration = cancellationToken.Register(static (state) => { ((ResponseContext)state!).Cancel(); }, this); diff --git a/test/YetAnotherHttpHandler.Test/Http2TestBase.cs b/test/YetAnotherHttpHandler.Test/Http2TestBase.cs index ffc6d1f..7066962 100644 --- a/test/YetAnotherHttpHandler.Test/Http2TestBase.cs +++ b/test/YetAnotherHttpHandler.Test/Http2TestBase.cs @@ -424,6 +424,65 @@ public async Task Cancel_Post_BeforeRequest() #endif } + + [ConditionalFact] + public async Task DisposeHandler_During_SendBuffer_Is_Full() + { + var runtimeHandle = NativeRuntime.Instance.Acquire(); + + // To prevent references remaining from local variables, make it a local function. + async Task RunAsync() + { + // Arrange + var httpHandler = CreateHandler(); + var httpClient = new HttpClient(httpHandler); + await using var server = await LaunchServerAsync(); + + // Act + var pipe = new Pipe(); + var writeTask = Task.Run(async () => + { + var buffer = new byte[1024 * 1024]; + while (true) + { + await pipe.Writer.WriteAsync(buffer); + } + }); + var content = new StreamContent(pipe.Reader.AsStream()); + content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/octet-stream"); + var request = new HttpRequestMessage(HttpMethod.Post, $"{server.BaseUri}/post-never-read") + { + Version = HttpVersion.Version20, + Content = content, + }; + var response = await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).WaitAsync(TimeoutToken); + await Task.Delay(TimeSpan.FromSeconds(5)); // Wait for the send buffer to overflow. + + // Decrease the reference count of manually held internal handles for direct observation. + NativeRuntime.Instance.Release(); + + // Dispose all related resources. + request.Dispose(); + response.Dispose(); + httpHandler.Dispose(); + httpClient.Dispose(); + } + + await RunAsync(); + + // Run Finalizer. + await Task.Delay(100); + GC.Collect(); + GC.Collect(); + GC.Collect(); + GC.Collect(); + await Task.Delay(100); + + // Assert + Assert.Equal(0, NativeRuntime.Instance._refCount); + Assert.True(runtimeHandle.IsClosed); + } + [ConditionalFact] public async Task Grpc_Unary() { diff --git a/test/YetAnotherHttpHandler.Test/TestServerForHttp1AndHttp2.cs b/test/YetAnotherHttpHandler.Test/TestServerForHttp1AndHttp2.cs index b2df606..f715077 100644 --- a/test/YetAnotherHttpHandler.Test/TestServerForHttp1AndHttp2.cs +++ b/test/YetAnotherHttpHandler.Test/TestServerForHttp1AndHttp2.cs @@ -164,6 +164,19 @@ public static WebApplication BuildApplication(WebApplicationBuilder builder) } return Results.Empty; }); + app.MapPost("/post-never-read", async (HttpContext httpContext) => + { + // Send status code and response headers. + httpContext.Response.Headers["x-header-1"] = "foo"; + httpContext.Response.StatusCode = (int)HttpStatusCode.OK; + await httpContext.Response.BodyWriter.FlushAsync(); + + while (!httpContext.RequestAborted.IsCancellationRequested) + { + await Task.Delay(100); + } + return Results.Empty; + }); // HTTP/2 app.MapGet("/error-reset", (HttpContext httpContext) => From c7deb795cde9c7d32119f5dc78f6914f4e386612 Mon Sep 17 00:00:00 2001 From: Mayuki Sawatari Date: Wed, 18 Sep 2024 15:54:04 +0900 Subject: [PATCH 2/2] Fix tests --- .../ClientCertificateTest.cs | 12 +-- test/YetAnotherHttpHandler.Test/Http1Test.cs | 9 +-- .../Http2ClearTextTest.cs | 6 +- test/YetAnotherHttpHandler.Test/Http2Test.cs | 12 +-- .../Http2TestBase.cs | 65 +--------------- .../YetAnotherHttpHandlerTest.cs | 74 ++++++++++++++++++- 6 files changed, 86 insertions(+), 92 deletions(-) diff --git a/test/YetAnotherHttpHandler.Test/ClientCertificateTest.cs b/test/YetAnotherHttpHandler.Test/ClientCertificateTest.cs index 80d3fd9..6ca7601 100644 --- a/test/YetAnotherHttpHandler.Test/ClientCertificateTest.cs +++ b/test/YetAnotherHttpHandler.Test/ClientCertificateTest.cs @@ -41,7 +41,7 @@ public async Task NotSet() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() + using var httpHandler = new YetAnotherHttpHandler() { //ClientAuthCertificates = File.ReadAllText("./Certificates/client.crt"), //ClientAuthKey = File.ReadAllText("./Certificates/client.key"), @@ -63,7 +63,7 @@ public async Task UseClientCertificate() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() + using var httpHandler = new YetAnotherHttpHandler() { ClientAuthCertificates = File.ReadAllText("./Certificates/client.crt"), ClientAuthKey = File.ReadAllText("./Certificates/client.key"), @@ -85,7 +85,7 @@ public async Task Invalid() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() + using var httpHandler = new YetAnotherHttpHandler() { ClientAuthCertificates = File.ReadAllText("./Certificates/client_unknown.crt"), // CN=unknown.example.com ClientAuthKey = File.ReadAllText("./Certificates/client_unknown.key"), @@ -106,7 +106,7 @@ public async Task Reference_SocketHttpHandler_NotSet() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new SocketsHttpHandler(); + using var httpHandler = new SocketsHttpHandler(); httpHandler.SslOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true; var httpClient = new HttpClient(httpHandler); @@ -123,7 +123,7 @@ public async Task Reference_SocketHttpHandler_UseClientCertificate() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new SocketsHttpHandler(); + using var httpHandler = new SocketsHttpHandler(); httpHandler.SslOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true; httpHandler.SslOptions.ClientCertificates = new X509CertificateCollection() { @@ -145,7 +145,7 @@ public async Task Reference_SocketHttpHandler_Invalid() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new SocketsHttpHandler(); + using var httpHandler = new SocketsHttpHandler(); httpHandler.SslOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, errors) => true; httpHandler.SslOptions.ClientCertificates = new X509CertificateCollection() { diff --git a/test/YetAnotherHttpHandler.Test/Http1Test.cs b/test/YetAnotherHttpHandler.Test/Http1Test.cs index 094b7b5..6b5c885 100644 --- a/test/YetAnotherHttpHandler.Test/Http1Test.cs +++ b/test/YetAnotherHttpHandler.Test/Http1Test.cs @@ -1,18 +1,11 @@ using System.IO.Pipelines; using System.Net; -using System.Runtime.ExceptionServices; -using _YetAnotherHttpHandler.Test.Helpers; using Xunit.Abstractions; -using Xunit.Sdk; namespace _YetAnotherHttpHandler.Test; -public class Http1Test : UseTestServerTestBase +public class Http1Test(ITestOutputHelper testOutputHelper) : UseTestServerTestBase(testOutputHelper) { - public Http1Test(ITestOutputHelper testOutputHelper) : base(testOutputHelper) - { - } - [Fact] public async Task FailedToConnect() { diff --git a/test/YetAnotherHttpHandler.Test/Http2ClearTextTest.cs b/test/YetAnotherHttpHandler.Test/Http2ClearTextTest.cs index ff93be7..37243e4 100644 --- a/test/YetAnotherHttpHandler.Test/Http2ClearTextTest.cs +++ b/test/YetAnotherHttpHandler.Test/Http2ClearTextTest.cs @@ -5,12 +5,8 @@ namespace _YetAnotherHttpHandler.Test; -public class Http2ClearTextTest : Http2TestBase +public class Http2ClearTextTest(ITestOutputHelper testOutputHelper) : Http2TestBase(testOutputHelper) { - public Http2ClearTextTest(ITestOutputHelper testOutputHelper) : base(testOutputHelper) - { - } - protected override YetAnotherHttpHandler CreateHandler() { return new YetAnotherHttpHandler() { Http2Only = true }; diff --git a/test/YetAnotherHttpHandler.Test/Http2Test.cs b/test/YetAnotherHttpHandler.Test/Http2Test.cs index e362686..f7517fa 100644 --- a/test/YetAnotherHttpHandler.Test/Http2Test.cs +++ b/test/YetAnotherHttpHandler.Test/Http2Test.cs @@ -8,12 +8,8 @@ namespace _YetAnotherHttpHandler.Test; [OSSkipCondition(OperatingSystems.MacOSX)] // .NET 7 or earlier does not support ALPN on macOS. -public class Http2Test : Http2TestBase +public class Http2Test(ITestOutputHelper testOutputHelper) : Http2TestBase(testOutputHelper) { - public Http2Test(ITestOutputHelper testOutputHelper) : base(testOutputHelper) - { - } - protected override YetAnotherHttpHandler CreateHandler() { // Use self-signed certificate for testing purpose. @@ -47,7 +43,7 @@ public async Task SelfSignedCertificate_NotTrusted() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() { SkipCertificateVerification = false }; // We need to verify server certificate. + using var httpHandler = new YetAnotherHttpHandler() { SkipCertificateVerification = false }; // We need to verify server certificate. var httpClient = new HttpClient(httpHandler); // Act @@ -63,7 +59,7 @@ public async Task SelfSignedCertificate_NotTrusted_SkipValidation() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() { SkipCertificateVerification = true }; + using var httpHandler = new YetAnotherHttpHandler() { SkipCertificateVerification = true }; var httpClient = new HttpClient(httpHandler); // Act @@ -80,7 +76,7 @@ public async Task SelfSignedCertificate_Trusted_CustomRootCA() { // Arrange await using var server = await LaunchServerAsync(); - var httpHandler = new YetAnotherHttpHandler() + using var httpHandler = new YetAnotherHttpHandler() { // We need to verify server certificate. SkipCertificateVerification = false, diff --git a/test/YetAnotherHttpHandler.Test/Http2TestBase.cs b/test/YetAnotherHttpHandler.Test/Http2TestBase.cs index 7066962..4ce0ed1 100644 --- a/test/YetAnotherHttpHandler.Test/Http2TestBase.cs +++ b/test/YetAnotherHttpHandler.Test/Http2TestBase.cs @@ -10,12 +10,8 @@ namespace _YetAnotherHttpHandler.Test; -public abstract class Http2TestBase : UseTestServerTestBase +public abstract class Http2TestBase(ITestOutputHelper testOutputHelper) : UseTestServerTestBase(testOutputHelper) { - protected Http2TestBase(ITestOutputHelper testOutputHelper) : base(testOutputHelper) - { - } - protected abstract YetAnotherHttpHandler CreateHandler(); protected abstract Task LaunchServerAsyncCore(Action? configure = null) where T : ITestServerBuilder; @@ -424,65 +420,6 @@ public async Task Cancel_Post_BeforeRequest() #endif } - - [ConditionalFact] - public async Task DisposeHandler_During_SendBuffer_Is_Full() - { - var runtimeHandle = NativeRuntime.Instance.Acquire(); - - // To prevent references remaining from local variables, make it a local function. - async Task RunAsync() - { - // Arrange - var httpHandler = CreateHandler(); - var httpClient = new HttpClient(httpHandler); - await using var server = await LaunchServerAsync(); - - // Act - var pipe = new Pipe(); - var writeTask = Task.Run(async () => - { - var buffer = new byte[1024 * 1024]; - while (true) - { - await pipe.Writer.WriteAsync(buffer); - } - }); - var content = new StreamContent(pipe.Reader.AsStream()); - content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/octet-stream"); - var request = new HttpRequestMessage(HttpMethod.Post, $"{server.BaseUri}/post-never-read") - { - Version = HttpVersion.Version20, - Content = content, - }; - var response = await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).WaitAsync(TimeoutToken); - await Task.Delay(TimeSpan.FromSeconds(5)); // Wait for the send buffer to overflow. - - // Decrease the reference count of manually held internal handles for direct observation. - NativeRuntime.Instance.Release(); - - // Dispose all related resources. - request.Dispose(); - response.Dispose(); - httpHandler.Dispose(); - httpClient.Dispose(); - } - - await RunAsync(); - - // Run Finalizer. - await Task.Delay(100); - GC.Collect(); - GC.Collect(); - GC.Collect(); - GC.Collect(); - await Task.Delay(100); - - // Assert - Assert.Equal(0, NativeRuntime.Instance._refCount); - Assert.True(runtimeHandle.IsClosed); - } - [ConditionalFact] public async Task Grpc_Unary() { diff --git a/test/YetAnotherHttpHandler.Test/YetAnotherHttpHandlerTest.cs b/test/YetAnotherHttpHandler.Test/YetAnotherHttpHandlerTest.cs index fdfb0d4..b967ad2 100644 --- a/test/YetAnotherHttpHandler.Test/YetAnotherHttpHandlerTest.cs +++ b/test/YetAnotherHttpHandler.Test/YetAnotherHttpHandlerTest.cs @@ -1,8 +1,16 @@ using Cysharp.Net.Http; +using System.IO.Pipelines; +using System.Net; +using System.Net.Http.Headers; +using Xunit.Abstractions; namespace _YetAnotherHttpHandler.Test; -public class YetAnotherHttpHandlerTest +[CollectionDefinition(nameof(YetAnotherHttpHandlerTest), DisableParallelization = true)] +public class YetAnotherHttpHandlerTestCollection; + +[Collection(nameof(YetAnotherHttpHandlerTest))] +public class YetAnotherHttpHandlerTest(ITestOutputHelper testOutputHelper) : UseTestServerTestBase(testOutputHelper) { [Fact] public async Task Disposed() @@ -18,4 +26,68 @@ public async Task Disposed() // Assert Assert.IsType(ex); } + + [Fact] + public async Task DisposeHandler_During_SendBuffer_Is_Full() + { + GC.Collect(); + GC.Collect(); + GC.Collect(); + GC.Collect(); + + var runtimeHandle = NativeRuntime.Instance.Acquire(); + + // To prevent references remaining from local variables, make it a local function. + async Task RunAsync() + { + // Arrange + var httpHandler = new YetAnotherHttpHandler() { Http2Only = true }; + var httpClient = new HttpClient(httpHandler); + await using var server = await LaunchServerAsync(TestWebAppServerListenMode.InsecureHttp2Only); + + // Act + var pipe = new Pipe(); + var writeTask = Task.Run(async () => + { + var buffer = new byte[1024 * 1024]; + while (true) + { + await pipe.Writer.WriteAsync(buffer); + } + }); + var content = new StreamContent(pipe.Reader.AsStream()); + content.Headers.ContentType = MediaTypeHeaderValue.Parse("application/octet-stream"); + var request = new HttpRequestMessage(HttpMethod.Post, $"{server.BaseUri}/post-never-read") + { + Version = HttpVersion.Version20, + Content = content, + }; + var response = await httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead).WaitAsync(TimeoutToken); + await Task.Delay(TimeSpan.FromSeconds(5)); // Wait for the send buffer to overflow. + + // Decrease the reference count of manually held internal handles for direct observation. + NativeRuntime.Instance.Release(); + + // Dispose all related resources. + request.Dispose(); + response.Dispose(); + httpHandler.Dispose(); + httpClient.Dispose(); + } + + await RunAsync(); + + // Run Finalizer. + await Task.Delay(100); + GC.Collect(); + GC.Collect(); + GC.Collect(); + GC.Collect(); + await Task.Delay(100); + + // Assert + Assert.Equal(0, NativeRuntime.Instance._refCount); + Assert.True(runtimeHandle.IsClosed); + } + } \ No newline at end of file