Server and client are synchronized via TaskCompletionSource #35569
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
Can you run the outerloop test to verify this fix? |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Is this test long running (I see SlowTestStream)? If not, should this be moved to innerloop/outerloop reason updated? |
|
It might take several seconds to complete, so I'd rather keep it in outerloop. |
| // Delay for a bit to ensure that the RST_STREAM for the previous request is sent before the next request starts. | ||
| await Task.Delay(2000); | ||
| // Wait until the RST_STREAM for the previous request is received before the next request starts. | ||
| await rstReceived.Task.TimeoutAfter(TimeSpan.FromSeconds(60)); |
There was a problem hiding this comment.
60
Do we have a standard timeout constant defined anywhere that we could use for consistency (and the ability to change all timeouts at once later) instead of hardcoding the 60?
There was a problem hiding this comment.
I already tried searching for such constant, but with no luck.
There was a problem hiding this comment.
On the second thought, I found Http2LoopbackServer.Timeout, but not sure if it's a right thing to use it here.
There was a problem hiding this comment.
there is TestConfiguration.PassingTestTimeoutMilliseconds but that may be too long. That generally puts cap on test execution. I think using loopback timeout would be fine in this case as it does not need to precise, right @alnikola? Maybe we can add something DefaultTimeout to loopback and than use it when appropriate. (like the Http2LoopbackServer.Timeout) That would allows us to adjust it globally if we ever need to on slow platforms.
There was a problem hiding this comment.
I will try to find all places where we use an explicit timeout and put there some well-known value (e.g. TestConfiguration.PassingTestTimeoutMilliseconds or Http2LoopbackServer.Timeout). Will create a separate PR for this.
ghost
locked as resolved and limited conversation to collaborators
Http2_PendingSend_SendsReset test fails because client sends the second request with EndOfStream flag sooner than the server reads the last RstStream frame of the first request. This is caused by too short delay before the second request is made on the client. This PR replaces Task.Delay with a synchronization via TaskCompletionSource.
Fixes #2131