make Quic AcceptStreamAsync concurrent safe

[wfurt]

I found this while investigating #55249
While I could rarely reproduce the reported issue, I would often see strange timeout.
With the current code, running AcceptStreamAsync in parallel is not safe.
Thanks to @stephentoub who pointed the right place when we would throw OperationCancelledException without token expiring.
With the change, I'm yet to see the InvalidOperationException. (not really sure why)

contributes to #55249
I'm not sure if this really matters to runtime tests but the local runs seems happier. (not enough samples yet)

cc: @JamesNK

[ghost]

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

Issue Details

---

I found this while investigating #55249
While I could rarely reproduce the reported issue, I would often see strange timeout.
With the current code, running AcceptStreamAsync in parallel is not safe.
Thanks to @stephentoub who pointed the right place when we would throw OperationCancelledException without token expiring.
With the change, I'm yet to see the InvalidOperationException. (not really sure why)

contributes to #55249
I'm not sure if this really matters to runtime tests but the local runs seems happier. (not enough samples yet)

cc: @JamesNK

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Quic
Milestone:	-

[JamesNK]

Ah, I didn't realize AcceptStreamAsync wasn't thread-safe. In the real-world, Kestrel will only have one call to AcceptStreamAsync active at a time.

In some errors from that test the exception said that multiple ReadAsync calls at a time weren't supported. I guess in that situation one QuicStream was being returned multiple times.

Up to you whether to make AcceptStreamAsync thread-safe or throw a more user-friendly error on multiple concurrent calls (like QuicStream.ReadAsync does). I can modify the test to be more real-world.

[JamesNK]

If I update the test to lock the code that calls AcceptStreamAsync then it passes 100%

await asyncLock.WaitAsync();
try
{
    serverStream = await requestState.ServerConnection.AcceptAsync();
}
finally
{
    asyncLock.Release();
}

[wfurt]

If I update the test to lock the code that calls AcceptStreamAsync then it passes 100%

The accept failure should have no impact on the reading state. I will proceed with this change and I'll keep it watching for a while. I suspect that some other issue may be lurking out there - we are just lucky because of changed timing.

[CarnaViire]

With old SingleReader=true, it still could not be that two concurrent readers would get the same stream instance from the queue, could it? 🤔

[wfurt]

no, I don't think so @CarnaViire. This what the Channel is for to provide atomic operations.