Don't cache CanReuse value

[sebastienros]

Fixes #34768

Regression?

• Yes
• No

Risk

• High
• Medium
• Low

Worst case is that Http2 streams are pooled less often.

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

[Tratcher]

Would resetting the value to false in OnReset accomplish the same goal?

[halter73]

The line on 44 was effectively doing that before. The issue was that !_connectionAborted && HasResponseCompleted was being evaluated too early on a background thread and a RST_STREAM frame could still be processed and abort the stream before pooling. Now CanReuse is evaluated once in the main frame-processing loop right before it's pooled or not.

[JamesNK]

Please add a unit test. This is subtle behavior and we want to ensure it isn't regressed in the future.

Where is _connectionAborted set to true? Will it always happen before the stream is pooled? I want to make sure there is no chance that a stream is pooled and then some code aborts it while it is in the pool.

[sebastienros]

Where is _connectionAborted set to true?

When the RST_STREAM is processed.

Edit:
https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L731

[halter73]

Please add a unit test.

I would also love that if you find a good way to do it @JamesNK. The problem is the RST_STREAM frame needs to be processed between Http2Stream.CompleteStream(bool) executing on a background thread and Http2Connection.UpdateCompletedStreams() running again.

This would be easy enough to do by adding some sort of event/resettable-Task we could hook in the tests when Http2Connection.OnStreamCompleted() is called and only setting the RST_STREAM frame then, but the problem there is that UpdateCompletedStreams() runs immediately after await Input.ReadAsync() completes without processing any incoming frames. So we'd also need a way to force processing frames without calling UpdateCompletedStreams() to simulate Http2Connection frame-processing loop running concurrently with Http2Stream.CompleteStream(bool). I'm not sure it's worth it.

[JamesNK]

Ok. If we can't add a unit test then we should comment. Something like:

CanReuse must be evaluated on the main frame-processing looping after the stream is removed from the connection's active streams collection. This is required because a RST_STREAM frame could arrive after the END_STREAM flag is received. Only once the stream is removed from the connection's active stream collection can no longer be reset, and is safe to evaluate for pooling.

[davidfowl]

Nice catch!

[davidfowl]

Why is this is main and not rc1?

[sebastienros]

@davidfowl merge to main, then backport to rc1 (standard procedure I believe)

[sebastienros]

/backport to release/6.0-rc1

[github-actions]

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1164353234