Fix idle HTTP/2 connection KeepAliveTimeout teardown

[MihaZupan]

Fixes #95621 for 9.0.
This was a regression introduced in 8.0 by #90094.

---

#95621 reports that the following exception was thrown in a timer callback, which in turn crashed the process:

System.Threading.Channels.ChannelClosedException: The channel has been closed.
at System.Net.Http.Http2Connection.FinalTeardown()
at System.Net.Http.Http2Connection.Shutdown()
at System.Net.Http.Http2Connection.Abort(Exception abortException)
at System.Net.Http.Http2Connection.HeartBeat()
at System.Net.Http.HttpConnectionPool.HeartBeat()
at System.Net.Http.HttpConnectionPoolManager.HeartBeat()
at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool)
at System.Threading.TimerQueue.FireNextTimers()
at System.Threading.ThreadPoolWorkQueue.Dispatch()
at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

It is clear that FinalTeardown was somehow called twice.

Looking at the code involved in shutting down an HTTP/2 connection and removing unrelated things, it looks like this

bool _shutdown;

Abort()
{
    lock (SyncObject)
        Shutdown();
}

Dispose()
{
    lock (SyncObject)
        Shutdown();
}

Shutdown()
{
    if (!_shutdown)
    {
        InvalidateHttp2Connection(this);

        _shutdown = true;

        if (_streamsInUse == 0)
            FinalTeardown();
    }
}

InvalidateHttp2Connection(connection)
{
    if (TryRemoveConnectionFromPool())
        connection.Dispose();
}

// This should be called exactly once
void FinalTeardown() => _writeChannel.Writer.Complete();

Shutdown calls InvalidateHttp2Connection, which may call back into Dispose, which calls into Shutdown.
Because we set _shutdown after calling InvalidateHttp2Connection, we entered the if (!_shutdown) block twice (on the same call stack).
If the _streamsInUse == 0 condition holds, we will call into FinalTeardown twice.

This effectively means that if an idle HTTP/2 connection hits the KeepAliveTimeout, we'll crash the process.

The HttpKeepAlivePingPolicy_Always_NoPingResponseBetweenStreams_SecondRequestShouldFail test was meant to test this condition, but sadly it was always sending the second request on the client, so we weren't exercising the _streamsInUse == 0 condition.
After fixing the test, it is a 100 % repro for the reported issue.

The product change in this PR moves the toggling of the _shutdown flag before the call to InvalidateHttp2Connection.

[ghost]

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

Issue Details

---

Fixes #95621 for 9.0.
This was a regression introduced in 8.0 by #90094.

---

#95621 reports that the following exception was thrown in a timer callback, which in turn crashed the process:

System.Threading.Channels.ChannelClosedException: The channel has been closed.
at System.Net.Http.Http2Connection.FinalTeardown()
at System.Net.Http.Http2Connection.Shutdown()
at System.Net.Http.Http2Connection.Abort(Exception abortException)
at System.Net.Http.Http2Connection.HeartBeat()
at System.Net.Http.HttpConnectionPool.HeartBeat()
at System.Net.Http.HttpConnectionPoolManager.HeartBeat()
at System.Threading.TimerQueueTimer.Fire(Boolean isThreadPool)
at System.Threading.TimerQueue.FireNextTimers()
at System.Threading.ThreadPoolWorkQueue.Dispatch()
at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

It is clear that FinalTeardown was somehow called twice.

Looking at the code involved in shutting down an HTTP/2 connection and removing unrelated things, it looks like this

bool _shutdown;

Abort()
{
    lock (SyncObject)
        Shutdown();
}

Dispose()
{
    lock (SyncObject)
        Shutdown();
}

Shutdown()
{
    if (!_shutdown)
    {
        InvalidateHttp2Connection(this);

        _shutdown = true;

        if (_streamsInUse == 0)
            FinalTeardown();
    }
}

InvalidateHttp2Connection(connection)
{
    if (TryRemoveConnectionFromPool())
        connection.Dispose();
}

// This should be called exactly once
void FinalTeardown() => _writeChannel.Writer.Complete();

Shutdown calls InvalidateHttp2Connection, which may call back into Dispose, which calls into Shutdown.
Because we set _shutdown after calling InvalidateHttp2Connection, we entered the if (!_shutdown) block twice (on the same call stack).
If the _streamsInUse == 0 condition holds, we will call into FinalTeardown twice.

This effectively means that if an idle HTTP/2 connection hits the KeepAliveTimeout, we'll crash the process.

The HttpKeepAlivePingPolicy_Always_NoPingResponseBetweenStreams_SecondRequestShouldFail test was meant to test this condition, but sadly it was always sending the second request on the client, so we weren't exercising the _streamsInUse == 0 condition.
After fixing the test, it is a 100 % repro for the reported issue.

The product change in this PR moves the toggling of the _shutdown flag before the call to InvalidateHttp2Connection.

Author:	MihaZupan
Assignees:	MihaZupan
Labels:	area-System.Net.Http
Milestone:	9.0.0

[wfurt]

LGTM

[MihaZupan]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[MihaZupan]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7256023935