Skip to content

Conversation

@antonfirsov
Copy link
Contributor

@antonfirsov antonfirsov commented Jan 28, 2021

Fixes #47561.

#47229 introduced a test with a timing race. In sync case Dispose may happen before the operation is started. (Thread pool starvation defers Task.Run() stuff in SocketTestHelper?)

@geoffkizer @wfurt PTAL

@ghost
Copy link

ghost commented Jan 28, 2021

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

Issue Details

Fixes #47561.

There is a timing race in the test. In sync case Dispose may happen before the operation is started. (Thread pool starvation defers Task.Run() stuff in SocketTestHelper?)

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

@antonfirsov antonfirsov requested a review from a team January 28, 2021 11:21
@antonfirsov
Copy link
Contributor Author

Test failure is #43928.


Task receiveTask = ReceiveMessageFromAsync(socket, new byte[1], GetGetDummyTestEndpoint());
await Task.Delay(msDelay);
msDelay *= 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the msDelay chages hare. I don't see it used laster. Also the seems generally problematic as the time needed can vary greatly across platforms. Would it work if we simply accept either exception if we feel that should be expected?

Copy link
Contributor Author

@antonfirsov antonfirsov Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see it used later.

The test is being retried 10 times in the sync case, doubling the delay time on each failure:

await RetryHelper.ExecuteAsync(() => RunTestAsync(), maxAttempts: 10, retryWhen: e => e is XunitException);

the time needed can vary greatly across platforms

The retry mechanism is here to address this. Most of the times the test will succeed within the 100ms delay. On slow platforms / high CI load, first attempts will fail with an ObjectDisposedException, but eventually one of the subsequent retries will observe a SocketException

I adapted this practice from an existing test introduced in dotnet/corefx#38804:

// We try this a couple of times to deal with a timing race: if the Dispose happens
// before the operation is started, the peer won't see a ConnectionReset SocketException and we won't
// see a SocketException either.
int msDelay = 100;
await RetryHelper.ExecuteAsync(async () =>
{

@geoffkizer
Copy link
Contributor

I hate that the test param is named closeOrDispose, that's a terrible name for a bool param. But that predates this PR so not something to block on.

@geoffkizer geoffkizer merged commit d830a7d into dotnet:master Feb 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

4 participants