Thread.Sleep() vs Task.Delay().Wait()

Question

Codebase was like this in a non-async¹ method:

Thread.Sleep(500);

A colleague refactored it to be like this:

Task.Delay(500).Wait();

Given the relative small delay, is really an advantage to use the async version (Task.Delay()) ?
Does it change if the delay is a bunch of millisecond or a bunch of seconds ?
More important... Is not the .Wait() defeating he purpose of making an async ?

To provide more context, this is executes in a background process that runs an average of every minute.

Isn't this second approach just wasting more resources ? (as some comments here seems to indicate)

[Edit]
The await is not used in this case because the method is NOT async.
My colleague prefers Task.Delay(N).Wait() to Thread.Sleep(N).
Where the method is async he used await and not .Wait().

In this specific case the method is called when an API endpoint is hit and send a request to an external service and tries 3 or 5 times (with a delay) until it gets the result (kind of polling).

¹ _{Originally the method was mentioned to be async.}

Answer 1

await Task.Delay(500);

You should almost never ^† use .Wait(), but Task.Delay(...).Wait() is a special circle of "nope".

Thread.Sleep(...) is ok if you are happy with blocking the current thread; you shouldn't do that on a concurrent application (such as a server), though.

---

^† yes, there are scenarios where it is acceptable or even necessary; but if you have to ask: "don't" is a good default answer; any full answer on this involves multiple pages of context, nuance, etc.

Answer 2

Codebase was like this:

Thread.Sleep(500);

A colleague refactored it to be like this:

Task.Delay(500).Wait();

Your colleague's refactoring constitutes neither an improvement nor a regression. Both commands block the current thread for 500 milliseconds. Functionally there is no difference between them. If one is more precise than the other (in the order of a milliseconds), it's probably the Thread.Sleep(500), but I haven't tested experimentally this theory.

The mechanism that blocks the current thread is not exactly the same. In the case of the Thread.Sleep there is a direct call to the Interop.Kernel32.Sleep API (source code). In the case of Task.Delay+Wait there is much more going on. It involves the .NET TimerQueue infrastructure, a ManualResetEventSlim for blocking the current thread until a signal comes from the said infrastructure, and eventually deep in this infrastructure there should be a Interop.Kernel32.Sleep as well that manages the activity of the dedicated .NET thread that ticks the timers. But in the grand scheme of things all these are happening very fast, and you shouldn't see any measurable negative effect in performance by using your colleague's approach.

It is mostly painful for the eyes of the reviewers though, to see a simple line of code replaced by a less simple line of code that does the same thing, but requires significantly more mental effort to comprehend. Including the mental effort associated with querying the intentions behind this unwarranted complexity.

---

Caution: As Marc Gravell mentioned in a comment, the Task.Delay(500).Wait(); relies on the availability of a ThreadPool thread to complete the Task.Delay task that signals the awakening of the blocked thread. So in a scenario where the ThreadPool is saturated, the Task.Delay(500).Wait(); can become remarkably innacurate, extending the delay time by many seconds. Theoretically it could even cause a deadlock in a scenario where the ThreadPool has reached its maximum size, and the saturation incident can't be resolved by injecting more threads. The maximum size of the ThreadPool is normally pretty high (32,767 in my machine for a console app), so this scenario is quite unlikely to occur in practice.