Call Task.Yield() instead of Thread.Sleep() to yield control about asyncusageanalyzers

[sharwell]

await Task.Delay(...) is a drop-in replacement for Thread.Sleep() within an async method. I believe Task.Yield is used for other purposes.

[pdelvo]

Task.Yield() returns a task that if you ask it if it is finished will return false, and then finish immediately. await will check if a Task is already finished and it will not give the control back to the caller if it is to reduce the number of context switches. So Task.Yield() will make sure the method returns and a context switch is going to happen.

[sharwell]

So this rule should actually be an analyzer that detects uses of Thread.Sleep(...) in an asynchronous method, and replaces them with await Task.Delay(...). I'm not sure what to call it, but overall it gets a 👍 from me.

[nbarbettini]

👍 From me as well.

[tmaczynski]

I plan to implement this analyzer after I finish implementing issue 1562 from StyleCopAnalyzers.

I think that this diagnostic and code fix should be a little bit more complex since async code might call other methods which use Thread.Sleep(...) and if those methods are private and are used only from async methods, they should be fixed as well.

[sharwell]

@tmaczynski You could just make two different diagnostics.

1. Thread.Sleep should not be used inside of asynchronous methods
2. Thread.Sleep should not be used anywhere

This simplifies the analysis a bit.

[tmaczynski]

@sharwell I've created a pull request #50 with two diagnostics as you suggested.

[tmaczynski]

Could you change the tag of this issue - it should be "pull request" instead of "up for grabs". I looking forward to have this PR reviewed - extension methods that I added might help me with issue #1.

[JohanLarsson]

There are a few special cases with Thread.Sleep iirc.
Thread.Sleep(0) is similar to Task.Yield.
Thread.Sleep(1) is also similar to Task.Yield but forces context switch.

Above is from broken memory.

[tmaczynski]

@JohanLarsson
Ad 1:

You're right that Thread.Sleep(0) is similar to await Task.Yield(), not await Task.Delay(0). MSDN doc of Sleep method says that:

If the value of the millisecondsTimeout argument is zero, the thread relinquishes the remainder of its time slice to any thread of equal priority that is ready to run.

Documentation of Task.Delay(....) do not have a special case for 0 , what's more the reference source (line 5878) indicates that Task.Delay(0, cancellationToken) returns Task.CompletedTask which means that awaiting that method can execute synchronously.

Semantic of await Task.Yield() seems to be close to that of Thread.Sleep(0). I guess that refactoring Thread.Sleep(0) to await Task.Yield() might introduce some regression in some edge cases (e.g. when code relies on affinity of the code that is scheduled after sleep), but I think that such refactoring is correct in 99% of cases.

Ad 2:

I don’t think that Thread.Sleep(1) is another special case - it’s simply the shortest possible sleep (far below default length of a Window timeslice which is ~15milliseconds), but await Task.Delay(1) works in similar way [source: 1].

I will add a special case for Thread.Sleep(0) shortly. In the meantime, more feedback is welcomed.

[1] https://msdn.microsoft.com/en-us/library/hh194873(v=vs.110).aspx :

This method depends on the system clock. This means that the time delay will approximately equal the resolution of the system clock if the millisecondsDelay argument is less than the resolution of the system clock, which is approximately 15 milliseconds on Windows systems.

[tmaczynski]

@JohanLarsson
I added added code which changes Thread.Sleep(0) and Thread.Sleep(TimeSpan.Zero) to await Task.Yield().

[tmaczynski]

Could you review my PR #50?