[ManickaP]

Likely timing issue where closing the connection was processed faster than the stream data receive. Removing the bigger buffer from the equation to minimize the probability of this happening.

Fixes #135843

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[wfurt]

is #135843 test bug? I'm OK with the change in general but wondering if we are loosing test coverage or masking some other problem.

[ManickaP]

AFAICS, it's a timing issue in the test (I have high confidence, but not bulletproof data confirming it). And I don't think we can make the test 100% predictable either.

When one side closes the connection, the peer will also close all associated streams with it. If we receive the stream event for connection closure and process it faster than the user code checks ReadsClosed, it will see it as completed (unsuccessfully). Basically, it will manifest as in the else branch:

runtime/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicStreamTests.cs

Lines 1367 to 1378 in 24543dd

	else
	{
	var ex = await AssertThrowsQuicExceptionAsync(expectedError, () => stream.ReadsClosed);
	if (expectedError == QuicError.OperationAborted)
	{
	Assert.Null(ex.ApplicationErrorCode);
	}
	else
	{
	Assert.Equal(expectedErrorCode, ex.ApplicationErrorCode);
	}
	}

If we'd go to the extremes of possibilities, we cannot say with 100% predictability which of the if / else branches happens for anything that's <= BufferSize. For bigger, we know it will be the else branch.

And I cannot add another set of semaphores, because we do want to test the behavior after the connection closure.

Honestly, I'm just trying to lower the probability of flakiness here. The failure happens once in a blue moon. If you don't like this change, I'm just gonna close the PR and leave the issue open to catch the few occurrences for BA. This is not worth more effort.