Socket tests: don't retry CanceledByDispose tests on timeout #42725
Conversation
These retries cause masking cases where the operation and dispose Task are never completing.
|
Tagging subscribers to this area: @dotnet/ncl |
There was a problem hiding this comment.
Wouldn't it be better to extend RetryHelper to detect fatal failures?
See:
1339f52
| @@ -127,7 +127,8 @@ public async Task ConnectGetsCanceledByDispose() | |||
| // We try this a couple of times to deal with a timing race: if the Dispose happens | |||
| // before the operation is started, we won't see a SocketException. | |||
| int msDelay = 100; | |||
| await RetryHelper.ExecuteAsync(async () => | |||
| int retries = 10; | |||
| while (true) | |||
There was a problem hiding this comment.
catch (SocketException se)
{
// On connection timeout, retry.
Assert.NotEqual(SocketError.TimedOut, se.SocketErrorCode);
localSocketError = se.SocketErrorCode;
}The test fails here on Windows. Should do some more research on SocketError.TimedOut cases, but I think we should keep retrying here.
Yes, this is only meant to fix the test bug. |
src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ssl.c
Outdated
Show resolved
Hide resolved
|
@tmds what do you think about my idea regarding |
I think you need to look at multiple tests to know whether it makes sense to extend the RetryHelper for this pattern. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Part of #42725. |
ghost
locked as resolved and limited conversation to collaborators
These retries cause masking cases where the operation and
dispose Task are never completing.
Fixes #42686 (comment)
@MihaZupan @antonfirsov ptal