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

Improve server GOAWAY exception message in HttpClient #1966

Open aik-jahoda opened 4 years ago

aik-jahoda commented 4 years ago

When there is a protocol error on client-side we don't handle server GOAWAY or premature stream close correctly. Instead, we have not a clear exception:

System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.IO.IOException: The request was aborted.
 ---> System.IO.IOException: The response ended prematurely, with at least 9 additional bytes expected.
   at System.Net.Http.Http2Connection.ReadAtLeastAsync(Stream stream, Memory`1 buffer, Int32 minReadBytes) in /Users/janjahoda/workspace/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs:line 1892
   at System.Net.Http.Http2Connection.EnsureIncomingBytesAsync(Int32 minReadBytes) in /Users/janjahoda/workspace/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs:line 176
   at System.Net.Http.Http2Connection.ReadFrameAsync(Boolean initialFrame) in /Users/janjahoda/workspace/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs:line 207
   at System.Net.Http.Http2Connection.ProcessIncomingFramesAsync() in /Users/janjahoda/workspace/runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs:line 245
stephentoub commented 4 years ago

we don't handle ... premature stream close correctly. Instead, we have not a clear exception

The exception is highlighting that it tried to read the next frame header but couldn't because the stream closed unexpectedly. We could improve the error message, of course, but isn't this handling it correctly?

GOAWAY

You see the same exception when the server sends GOAWAY?

scalablecory commented 4 years ago

When there is a protocol error

@aik-jahoda Are you saying that when we abort the connection due to a protocol error in request A, we aren't shutting down all the various tasks and so request B gets a "response ended prematurely" error rather than the same protocol error?

A repro would be helpful here.

karelz commented 4 years ago

@aik-jahoda can you post the repro here?

karelz commented 4 years ago

@aik-jahoda ping?

aik-jahoda commented 4 years ago

The error message is confusing as we receive GOAWAY and we precisely know the stream was closed. The message looks like we are still expecting bytes.

Proposed message: "The response ended prematurely, GOAWAY received"

Repro is here ```c# using System; using System.Linq; using System.Net; using System.Net.Http; using System.Threading.Tasks; using Microsoft.AspNetCore; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Hosting.Server.Features; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.Extensions.Hosting; namespace gh1966 { public class Program { private const string UNENCRYPTED_HTTP2_ENV_VAR = "DOTNET_SYSTEM_NET_HTTP_SOCKETSHTTPHANDLER_HTTP2UNENCRYPTEDSUPPORT"; public static async Task Main(string[] args) { var host = CreateHostBuilder(args).Build(); host.Start(); string address = host.ServerFeatures .Get() .Addresses .First(); Console.WriteLine(address); Console.ReadLine(); Environment.SetEnvironmentVariable(UNENCRYPTED_HTTP2_ENV_VAR, "1"); SocketsHttpHandler handler = new SocketsHttpHandler(); handler.SslOptions.RemoteCertificateValidationCallback = delegate { return true; }; HttpClient client = new HttpClient(handler) { DefaultRequestVersion = HttpVersion.Version20 }; client.DefaultRequestHeaders.TryAddWithoutValidation(new string('*', 50000), "0fsd"); string result = await client.GetStringAsync(address); Console.WriteLine(result); host.WaitForShutdown(); } private static void MapRoutes(IEndpointRouteBuilder endpoints) { var head = new[] { "HEAD" }; endpoints.MapGet("/", async context => { await context.Response.WriteAsync("ok " + string.Join(",", context.Request.Headers.Select(h => h.ToString()))); }); } public static IWebHostBuilder CreateHostBuilder(string[] args) => WebHost.CreateDefaultBuilder(args) .UseKestrel(options => options.Listen(IPAddress.Loopback, 0, (ListenOptions listenOptions) => { listenOptions.Protocols = HttpProtocols.Http2; })) .Configure(app => { app.UseRouting(); app.UseEndpoints(MapRoutes); }); } } ```
karelz commented 4 years ago

I see, you are proposing a message change. In that case, I recommend to mention 'server' -- e.g. "Server ended the response prematurely". We do not expect API users to be familiar with details of the HTTP/2 protocol IMO.