search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.12k stars 4.7k forks source link

Synchronous API of HttpClient isn't totally synchronous and cannot accept a synchronous-only stream from ConnectCallback #52413

Closed jhudsoncedaron closed 3 years ago

jhudsoncedaron commented 3 years ago

I'm going to state outright that I have a very limited idea what's going on here or why. I gave HttpClient a non-async-capable stream and only called the syncronous API and HttpClient still tried to perform async operations on it. I think the application kept running only because HttpClient would catch the exception somewhere and do something I'm having trouble guessing to recover from the situation. If I had to guess I'd say it needs to open one connection per request to recover from this.

If however I change the async methods to synchronous, the entire application deadlocks. HttpClient cannot accept arbitrary streams from ConnectCallback. There are too many kinds of streams that simply are not synchronous.

Description

I'm going to unmask part of our development code that demonstrates the problem. You won't be able to compile this, let alone run it. I don't have the toolset to figure out why it's behaving like this so I can't write a minimal repro right now.

            private static HttpClient HttpClientFactory()
            {
                var handler = new SocketsHttpHandler();
                handler.ConnectCallback = SyncConnectKeepaliveCallback;
                var client = new HttpClient(handler);
                client.DefaultRequestVersion = new Version(1, 1);
                client.Timeout = Timeout.InfiniteTimeSpan;
                return client;
            }

            public static ValueTask<Stream> SyncConnectKeepaliveCallback(SocketsHttpConnectionContext context, CancellationToken _)
            {
                var socket = Internet.ConnectToHost(context.DnsEndPoint.Host, context.DnsEndPoint.Port, ProtocolType.Tcp);
                // We actually P/Invoke here to set the keep alive time short. Not that it matters.
                socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true);
                return new ValueTask<Stream>(new AsyncFreeStream(new NetworkStream(socket, false), socket));
            }

        // Library packing this is not very useful due to the differing debug and release behavior
        // In debug we fail immediately. In release we try to work even though it won't work well.
        private class AsyncFreeStream : UnclosableStream
        {
            private readonly IDisposable ultimateBacker;

            public AsyncFreeStream(Stream immediateBacker, IDisposable ultimateBacker) : base(immediateBacker)
            {
                this.ultimateBacker = ultimateBacker;
            }

            public override Task<int> ReadAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default)
            {
#if DEBUG
System.Console.Error.WriteLine("I'm not supposed to be reachable.");
                throw new NotSupportedException("I'm not supposed to be reachable.");
#else
                return Task.FromResult(Read(buffer, offset, size));
#endif
            }

            public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
            {
#if DEBUG
System.Console.Error.WriteLine("I'm not supposed to be reachable.");
                throw new NotSupportedException("I'm not supposed to be reachable.");
#else
                return ValueTask.FromResult(base.Read(buffer.Span));
#endif
            }

            public override Task WriteAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default)
            {
#if DEBUG
System.Console.Error.WriteLine("I'm not supposed to be reachable.");
                throw new NotSupportedException("I'm not supposed to be reachable.");
#else
                Write(buffer, offset, size);
                return Task.CompletedTask;
#endif
            }

            public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)
            {
#if DEBUG
System.Console.Error.WriteLine("I'm not supposed to be reachable.");
                throw new NotSupportedException("I'm not supposed to be reachable.");
#else
                Write(buffer.Span);
                return ValueTask.CompletedTask;
#endif
            }

            public override void Close() => ultimateBacker?.Dispose();

            protected override void Dispose(bool disposing)
            {
                if (disposing) ultimateBacker?.Dispose();
            }

            public override ValueTask DisposeAsync()
            {
                ultimateBacker?.Dispose();
                return ValueTask.CompletedTask;
            }
        }

The primary purpose of wrapping-up NetworkStream like this is to detect potential deadlocks due to attempting async over sync over async. I wasn't expecting to find one inside of HttpClient itself.

Configuration

dotnet version 6.0.100-preview.3.21202.5

Regression?

No. Sync API never worked.

Other information

What I'm doing appears to be pretty pedestrian compared to stuff that was proposed for ConnectCallback to be able to do; like giving HttpClient a stream that's really a unix domain socket to a proxy server.

ghost commented 3 years ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
I'm going to state outright that I have a very limited idea what's going on here or why. I gave HttpClient a non-async-capable stream and only called the syncronous API and HttpClient still tried to perform async operations on it. I think the application kept running only because HttpClient would catch the exception somewhere and do something I'm having trouble guessing to recover from the situation. If I had to guess I'd say it needs to open one connection per request to recover from this. If however I change the async methods to synchronous, the entire application deadlocks. HttpClient cannot accept arbitrary streams from ConnectCallback. There are too many kinds of streams that simply are not synchronous. ### Description I'm going to unmask part of our development code that demonstrates the problem. You won't be able to compile this, let alone run it. I don't have the toolset to figure out why it's behaving like this so I can't write a minimal repro right now. ```` private static HttpClient HttpClientFactory() { var handler = new SocketsHttpHandler(); handler.ConnectCallback = SyncConnectKeepaliveCallback; var client = new HttpClient(handler); client.DefaultRequestVersion = new Version(1, 1); client.Timeout = Timeout.InfiniteTimeSpan; return client; } public static ValueTask SyncConnectKeepaliveCallback(SocketsHttpConnectionContext context, CancellationToken _) { var socket = Internet.ConnectToHost(context.DnsEndPoint.Host, context.DnsEndPoint.Port, ProtocolType.Tcp); // We actually P/Invoke here to set the keep alive time short. Not that it matters. socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true); return new ValueTask(new AsyncFreeStream(new NetworkStream(socket, false), socket)); } // Library packing this is not very useful due to the differing debug and release behavior // In debug we fail immediately. In release we try to work even though it won't work well. private class AsyncFreeStream : UnclosableStream { private readonly IDisposable ultimateBacker; public AsyncFreeStream(Stream immediateBacker, IDisposable ultimateBacker) : base(immediateBacker) { this.ultimateBacker = ultimateBacker; } public override Task ReadAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else return Task.FromResult(Read(buffer, offset, size)); #endif } public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else return ValueTask.FromResult(base.Read(buffer.Span)); #endif } public override Task WriteAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else Write(buffer, offset, size); return Task.CompletedTask; #endif } public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else Write(buffer.Span); return ValueTask.CompletedTask; #endif } public override void Close() => ultimateBacker?.Dispose(); protected override void Dispose(bool disposing) { if (disposing) ultimateBacker?.Dispose(); } public override ValueTask DisposeAsync() { ultimateBacker?.Dispose(); return ValueTask.CompletedTask; } } ```` ### Configuration dotnet version 6.0.100-preview.3.21202.5 ### Regression? No. Sync API never worked. ### Other information What I'm doing appears to be pretty pedestrian compared to stuff that was proposed for ConnectCallback to be able to do; like giving HttpClient a stream that's really a unix domain socket to a proxy server.
Author: jhudsoncedaron
Assignees: -
Labels: `area-System.Net`, `untriaged`
Milestone: -
jhudsoncedaron commented 3 years ago

(3 hours later)

I'm now prepared to prove that even if it is a background thread, that it is not possible for the threads making requests to reliably avoid deadlock. I considered use of [ThreadStatic] to verify just my own code, but that doesn't actually work because this really is a deadlock bug in HttpClient.

Assuming it is a background async job calling ReadAsync():

  1. Async job calls ReadAsync()
  2. thread returns to pool
  3. thread picks up incoming request in Kestrel
  4. thread calls API surface area, which descends through the synchronous intermediates
  5. deep in the stack, thread attempts a synchronous HttpClient call to the same server (essentially all of them)
  6. HttpClient sends the http request to that connection
  7. HttpClient tries to read the response from that connection
  8. NetworkStream notices the Read() is attempting to start while ReadAsync() is running and waits for ReadAsync() to finish
  9. The remaining worker threads pile lock on this worker thread (many possibilities)
  10. ReadAsync() finishes, the event listener thread enqueues the ContinueWith operation onto the task scheduler
  11. One second later, the task scheduler notices no progress can be made, tries to allocate a thread
  12. There's not enough free memory to allocate the thread stack, so the task scheduler goes to sleep. Even if it wakes up again, all threads are blocking so nothing's going to change.
ghost commented 3 years ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

Issue Details
I'm going to state outright that I have a very limited idea what's going on here or why. I gave HttpClient a non-async-capable stream and only called the syncronous API and HttpClient still tried to perform async operations on it. I think the application kept running only because HttpClient would catch the exception somewhere and do something I'm having trouble guessing to recover from the situation. If I had to guess I'd say it needs to open one connection per request to recover from this. If however I change the async methods to synchronous, the entire application deadlocks. HttpClient cannot accept arbitrary streams from ConnectCallback. There are too many kinds of streams that simply are not synchronous. ### Description I'm going to unmask part of our development code that demonstrates the problem. You won't be able to compile this, let alone run it. I don't have the toolset to figure out why it's behaving like this so I can't write a minimal repro right now. ```c# private static HttpClient HttpClientFactory() { var handler = new SocketsHttpHandler(); handler.ConnectCallback = SyncConnectKeepaliveCallback; var client = new HttpClient(handler); client.DefaultRequestVersion = new Version(1, 1); client.Timeout = Timeout.InfiniteTimeSpan; return client; } public static ValueTask SyncConnectKeepaliveCallback(SocketsHttpConnectionContext context, CancellationToken _) { var socket = Internet.ConnectToHost(context.DnsEndPoint.Host, context.DnsEndPoint.Port, ProtocolType.Tcp); // We actually P/Invoke here to set the keep alive time short. Not that it matters. socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.KeepAlive, true); return new ValueTask(new AsyncFreeStream(new NetworkStream(socket, false), socket)); } // Library packing this is not very useful due to the differing debug and release behavior // In debug we fail immediately. In release we try to work even though it won't work well. private class AsyncFreeStream : UnclosableStream { private readonly IDisposable ultimateBacker; public AsyncFreeStream(Stream immediateBacker, IDisposable ultimateBacker) : base(immediateBacker) { this.ultimateBacker = ultimateBacker; } public override Task ReadAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else return Task.FromResult(Read(buffer, offset, size)); #endif } public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else return ValueTask.FromResult(base.Read(buffer.Span)); #endif } public override Task WriteAsync(byte[] buffer, int offset, int size, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else Write(buffer, offset, size); return Task.CompletedTask; #endif } public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) { #if DEBUG System.Console.Error.WriteLine("I'm not supposed to be reachable."); throw new NotSupportedException("I'm not supposed to be reachable."); #else Write(buffer.Span); return ValueTask.CompletedTask; #endif } public override void Close() => ultimateBacker?.Dispose(); protected override void Dispose(bool disposing) { if (disposing) ultimateBacker?.Dispose(); } public override ValueTask DisposeAsync() { ultimateBacker?.Dispose(); return ValueTask.CompletedTask; } } ``` The primary purpose of wrapping-up NetworkStream like this is to detect potential deadlocks due to attempting async over sync over async. I wasn't expecting to find one inside of HttpClient itself. ### Configuration dotnet version 6.0.100-preview.3.21202.5 ### Regression? No. Sync API never worked. ### Other information What I'm doing appears to be pretty pedestrian compared to stuff that was proposed for ConnectCallback to be able to do; like giving HttpClient a stream that's really a unix domain socket to a proxy server.
Author: jhudsoncedaron
Assignees: -
Labels: `area-System.Net.Http`, `untriaged`
Milestone: -
karelz commented 3 years ago

Did you manage to take a look at callstack that tries to use async operations on the stream when it shouldn't? Which parts of HttpClient trigger it? We will likely need a repro to make this actionable. Or a callstack demonstrating where sync APIs accidentally call into async (your throwing wrapper should be able to catch it early, shouldn't it?).

How often does it happen? Can you reproduce it on single request? And without HttpClientFactory and other unnecessary components?

Also, I am curious why do you use synchronous APIs in your case? Do you need it? If yes, why?

cc @ManickaP

jhudsoncedaron commented 3 years ago

There are no unnecessary components here. The sample could hardly be any smaller and still inject the fault detection code.

The whole point of having a sync API is so you can call it while there's an unreachable await far above you on the stack; otherwise you could call the async methods with .GetAwaiter().GetResult(). The application is set up stream data into the HttpClient and stream data out on the application server, and to stream the response back in and stream it back out again into object model without ever materializing the (30+mb) buffers in RAM at once. According to Visual Studio's profiler the synchronous APIs almost never block once SQL Server starts returning results.

Long writeup here: https://github.com/dotnet/runtime/issues/51933#issuecomment-827898724

Environment.StackTrace info:

Interestingly, the first call to fail is not the first, not the second, but the third call. I never see the exceptions despite them being thrown with my own code higher up on the stack so HttpClient is doing something to recover.

I'm not supposed to be reachable.
   at System.Environment.get_StackTrace()
   at Cedaron.DataModel.Util.HttpClientManager.AsyncFreeStream.ReadAsync(Memory`1 buffer, CancellationToken cancellationToken) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.DataModel\Util\HttpClientManager.cs:line 165
   at System.Net.Http.HttpConnection.EnsureReadAheadAndPollRead()
   at System.Net.Http.HttpConnection.PollRead()
   at System.Net.Http.HttpConnectionPool.GetOrReserveHttp11ConnectionAsync(Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.GetHttpConnectionAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at System.Net.Http.HttpConnectionPool.GetHttpConnectionAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.GetConnectionAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithProxyAuthAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPoolManager.SendAsyncCore(HttpRequestMessage request, Uri proxyUri, Boolean async, Boolean doRequestAuth, Boolean isProxyConnect, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPoolManager.SendAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpMessageHandlerStage.Send(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.SocketsHttpHandler.Send(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpMessageInvoker.Send(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.Send(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.Send(HttpRequestMessage request)
   at Cedaron.DataModel.API.HTTPAPI.Invoke(Stream result, String name, Either`2 request, ISecurityProvider transportableSecurityProvider) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.DataModel.HTTPAPI\HTTPAPI.cs:line 66
   at Cedaron.DataModel.API.HTTPAPI.GetObjects(Stream result, Either`2 request, ISecurityProvider context) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.DataModel.HTTPAPI\HTTPAPI.cs:line 274
   at Cedaron.DataModel.API.ExecutionContext.GetPODObjects(IEnumerable`1 references) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.DataModel\API\ExecutionContext.cs:line 360
   at Cedaron.DataModel.API.ExecutionContext.GetObjects(IEnumerable`1 references) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.DataModel\API\ExecutionContext.cs:line 339
   at Cedaron.FormBuilder.Presentation.Services.ServerUserSettingService.GetSettingsObject(String key) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\ServerUserSettingService.cs:line 22
   at Cedaron.FormBuilder.Presentation.Services.ServerUserSettingService.LoadUserSetting(String key) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\ServerUserSettingService.cs:line 27
   at Cedaron.FormBuilder.Presentation.Services.UserLanguageChooser.get_LanguageId() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\UserLanguageChooser.cs:line 34
   at Cedaron.FormBuilder.Presentation.Services.UserLanguageChooser.GetLanguageId() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\UserLanguageChooser.cs:line 30
   at Cedaron.FormBuilder.Presentation.Services.BaseService.get_LanguageId() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\BaseService.cs:line 13
   at Cedaron.FormBuilder.Presentation.Services.SettingService.GetSystemSettings() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\SettingService.cs:line 268
   at Cedaron.FormBuilder.Presentation.Services.PatientService.GetPatientCount() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\PatientService.cs:line 239
   at Cedaron.FormBuilder.Presentation.Services.PatientService.GetPatientSearchViewModel() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Services\PatientService.cs:line 492
   at Cedaron.FormBuilder.Presentation.Controllers.PatientController.Search() in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Controllers\PatientController.cs:line 99
   at lambda_method11(Closure , Object , Object[] )
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.SyncActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeActionMethodAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAwaitedAsync()
   at Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAwaitedAsync()
   at Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeNextActionFilterAwaitedAsync()
   at Microsoft.AspNetCore.Mvc.Controller.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at Microsoft.AspNetCore.Mvc.Filters.ControllerActionFilter.OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextExceptionFilterAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeNextResourceFilter()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeAsync()
   at Microsoft.AspNetCore.Mvc.Routing.MvcRouteHandler.<>c__DisplayClass6_0.<RouteAsync>b__0(HttpContext c)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Builder.UseExtensions.<>c__DisplayClass0_2.<Use>b__2()
   at Cedaron.FormBuilder.Presentation.Infrastructure.RandomHeaderMiddleware.InsertRandomHeader(HttpContext context, Func`1 next) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Infrastructure\RandomHeaderMiddleware.cs:line 20
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Cedaron.FormBuilder.Presentation.Infrastructure.RandomHeaderMiddleware.InsertRandomHeader(HttpContext context, Func`1 next)
   at Microsoft.AspNetCore.Builder.UseExtensions.<>c__DisplayClass0_1.<Use>b__1(HttpContext context)
   at Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
   at Cedaron.FormBuilder.Presentation.Infrastructure.StatusCodePageMiddleware.Invoke(HttpContext context) in C:\Users\jhudson\dev\cento_forms3\src\Cedaron.FormBuilder.Presentation\Infrastructure\StatusCodePageMiddleware.cs:line 20
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Cedaron.FormBuilder.Presentation.Infrastructure.StatusCodePageMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.WebSockets.WebSocketMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionMiddleware.Invoke(HttpContext context)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Hosting.HostingApplication.ProcessRequestAsync(Context context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecutionContextCallback(Object s)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread threadPoolThread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecuteFromThreadPool(Thread threadPoolThread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
   at System.Threading.Thread.StartCallback()
ManickaP commented 3 years ago

https://github.com/dotnet/runtime/blob/236cb21e3c1992c8cee6935ce67e2125ac4687e8/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L165-L186

This is a combination of using sync HttpClient API (not recommended at all, especially if you already are in an async context) and using ConnectCallback.

The offending code: https://github.com/dotnet/runtime/blob/5fcf8cbd651e86105172653155595b80818fa9a5/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L142-L186

Every time we pick a connection from the pool, we issue read-ahead to make sure the connection hasn't been closed. In case of sync API we'd try to Poll the socket, but we don't have it if the connection is constructed manually in the ConnectCallback.

Honestly, there's not much we can do differently here. My suggestions is to go fully async, which is preferable, or make the ReadAsync work on the stream.

jhudsoncedaron commented 3 years ago

(not recommended at all, especially if you already are in an async context)

In which case you might as well withdraw the sync api altogether. The only reason for it to exist is to handle a call in a library that may or may not be on top of a far async.

This code is defective. I can imagine what would happen if I tried to push this down a tunnel. When you call ReadAsync() on an arbitrary stream, you must be prepared for the call to proceed synchronously anyway. (I tried it. Immediate deadlock should that ReadAsync become synchronous as happens on most synthetic streams.) Previously the code's been working because it's allowed to assume it has a NetworkStream.

Honestly, there's not much we can do differently here.

How about try sending the request to it and if the first Write()/WriteAsync() call hits a write error get another connection and try again.

jhudsoncedaron commented 3 years ago

Anyway, I want to replace the test harness code that actually opens up a listening connection with a ring buffer. There's no way that stream can provide Async() methods.

stephentoub commented 3 years ago

In which case you might as well withdraw the sync api altogether.

No. The sync API is generally fully synchronous, only falling back to synchronously blocking on an async operation in a few specific cases that you can also avoid if needed. There's no reason "to throw the baby out with the bathwater".

There are a few specific cases where we simply lack a good way to be fully synchronous. One such case is with Expect: 100-continue, and if it matters you can avoid setting that header (it's opt-in). Another is if you provide a custom connect stream that's not a NetworkStream such that we currently have no good way to access its socket to Poll when we try to determine whether the connection is still valid; you can avoid that by disabling connection lifetime management by setting the relevant properties on the handler to infinite.

jhudsoncedaron commented 3 years ago

How many more are there I have to find?

stephentoub commented 3 years ago

How about try sending the request to it and if the first Write()/WriteAsync() call hits a write error get another connection and try again.

We already do that. We also poll the connections in the pool so that, by default, we don't keep tons of connections around a server has closed. If your stream can't handle asynchronous operations (the 99.9% case of Send isn't that the stream can't handle async but that the caller wants it to be sync for thread management reasons), you can disable that connection management.

stephentoub commented 3 years ago

By design, the implementation expects a stream's async methods to work. The sync entrypoints are there to provide sync entrypoints to the caller, but again by design do not guarantee that all I/O will be sync all the way down to the kernel; whenever possible we do work synchronously in order to aid scalability, but there are cases where we will call async APIs on the Stream. In today's implementation, those include (this could change in the future):