dotnet / runtime

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

Http2Connection keeps the ExecutionContext of the first request alive #80232

Open MihaZupan opened 1 year ago

MihaZupan commented 1 year ago

As part of setting up the Http2Connection we spin up two background tasks for the read/write channel and in doing so we capture the execution context of the request that initiated the new connection.

We should do the SuppressFlow/RestoreFlow dance somewhere around Http2Connection.SetupAsync. This could also keep it alive.

Repro (note that the finalizer doesn't run until you dispose the client):

var client = new HttpClient
{
    DefaultRequestVersion = HttpVersion.Version20
};

await SendRequest(client);

_ = Task.Run(async () =>
{
    await Task.Delay(5_000);
    Console.WriteLine("Disposing the client");
    client.Dispose();
});

for (int i = 1; ; i++)
{
    Console.WriteLine(i);
    GC.Collect();
    GC.WaitForPendingFinalizers();
    await Task.Delay(1000);
}

async Task SendRequest(HttpClient client)
{
    var asyncLocal = new AsyncLocal<Finalizable>();
    asyncLocal.Value = new Finalizable();

    await client.GetStringAsync("https://httpbin.org/ip");
}

public sealed class Finalizable
{
    ~Finalizable() => Console.WriteLine("Finalizer");
}
ghost commented 1 year ago

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

Issue Details
As part of setting up the `Http2Connection` we spin up two background tasks for the read/write channel and in doing so we capture the execution context of the request that initiated the new connection. We should do the `SuppressFlow`/`RestoreFlow` dance before calling into [`Http2Connection.SetupAsync`](https://github.com/dotnet/runtime/blob/524045bb01369c26a0dfc349c1c6a8e26a8a9833/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs#L203). Repro (note that the finalizer doesn't run until you dispose the client): ```c# var client = new HttpClient { DefaultRequestVersion = HttpVersion.Version20 }; await SendRequest(client); _ = Task.Run(async () => { await Task.Delay(5_000); Console.WriteLine("Disposing the client"); client.Dispose(); }); for (int i = 1; ; i++) { Console.WriteLine(i); GC.Collect(); GC.WaitForPendingFinalizers(); await Task.Delay(1000); } async Task SendRequest(HttpClient client) { var asyncLocal = new AsyncLocal(); asyncLocal.Value = new Finalizable(); await client.GetStringAsync("https://httpbin.org/ip"); } public sealed class Finalizable { ~Finalizable() => Console.WriteLine("Finalizer"); } ```
Author: MihaZupan
Assignees: -
Labels: `bug`, `area-System.Net.Http`
Milestone: -
MihaZupan commented 1 year ago

Triage: This should be easy to fix, moving to 8.0.

MihaZupan commented 1 year ago

Judging by the code, we have the same issue with HTTP/3 as well.

We capture the ExecutionContext of the original request in these tasks inside Http3Connection's ctor: https://github.com/dotnet/runtime/blob/6ede02b93ce17925158486729be53cad34010c4f/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs#L102-L106