Is this good practice use of an async task with monitor and sleep?

Question

I am currently running an async task on polling (meaning this async task is called every 1s). All other items need to update that fast, with exception of my async task below. This code works, but I wonder if it's good practice?

Note: _monitor only gets used by DoAsync()

private readonly object _monitor = new object();

private void PolledEverySecond()
{
    _ = DoAsync(); // do this every 5 seconds
 
    // Other stuff
    GetNetworkState();
    GetCurrentVelocity();
    GetCurrentPosition();
    Etc;
}

private async Task DoAsync()
{
    await Task.Run(() =>
    {
        if (!Monitor.IsEntered(_monitor))
        {
            try
            {
                Monitor.Enter(_monitor);
                DoStuff();
            }
            finally
            {
                Thread.Sleep(5000);
                Monitor.Exit(_monitor);
            }
        }            
    });
}

The intention behind the Monitor.Enter/Monitor.Exit and the Thread.Sleep(5000). Is that DoAsync() does not get called every 1 second. I have a polling service that works great to update things and it's used by many of my ViewModels. But, in the case of DoAsync(), it is overkill to poll every second. Therefore, by making it async and using monitors, DoStuff() only gets called approximately every 5-6 seconds.

This isn't a comment on whether you should use `async` for this code, but it does look like your code has a race condition. `Monitor.IsEntered()` could return false, but before the next line of code is executed some other thread could enter the Monitor. You should probably be using `Montor.TryEnter()`, but without knowing the context it's difficult to say for sure. — Matthew Watson, Aug 10 '21 at 14:38
You shouldn't be blocking with either `Monitor` or `Thread.Sleep` on a threadpool thread anyway, create a new thread if you really have to do this — Charlieface, Aug 10 '21 at 14:43
Linked as duplicate: [When correctly use Task.Run and when just async-await](https://stackoverflow.com/questions/68728715/is-this-good-practice-use-of-an-async-task-with-monitor-and-sleep) — Theodor Zoulias, Aug 10 '21 at 15:00
"*All other items need to update that fast, with exception of my async task below.*": What are the other items? Where else do you use the `_monitor` as a locker? Could you include more info about the general problem that you are trying to solve? It's unlikely that you'll get any useful advice with so little context given. — Theodor Zoulias, Aug 10 '21 at 15:05
Thank you for the recommendation @TheodorZoulias. I have included some more info. — JP Garza, Aug 10 '21 at 16:35
Could you explain the desirable functionality of the `DoAsync` method? What's the intention behind the `Monitor.Enter`/`Monitor.Exit` and the `Thread.Sleep(5000);`? — Theodor Zoulias, Aug 10 '21 at 16:35
Is there any reason that you can't invoke the `DoStuff()` periodically using a separate `Timer` that has a 5 sec interval, independently from the `Other stuff`? — Theodor Zoulias, Aug 10 '21 at 16:41
@TheodorZoulias I have added my intention to the question. Hope this is useful, thanks. — JP Garza, Aug 10 '21 at 16:46

Theodor Zoulias · Accepted Answer · 2021-08-10T18:49:29.840

There are two problems with your current code, a race condition between the Monitor.IsEntered and Monitor.Enter calls, and the blocking of a ThreadPool thread with the Thread.Sleep(5000) call. Blocking ThreadPool threads is not consider a good practice, because it may result to the saturation of the pool. A saturated ThreadPool cannot respond immediately to requests for work, so the program becomes less responsive. Also in this case the ThreadPool has to inject more threads in the pool, resulting to increased memory consumption (each thread requires at least 1 MB of memory).

My suggestion is to switch from the blocking Thread.Sleep to the asynchronous (non-blocking) Task.Delay method, and also to switch from the thread-affine Monitor to the thread-agnostic SemaphoreSlim. In order to check the availability and acquire the semaphore as an atomic operation, you could use the Wait method, passing zero milliseconds as argument:

private readonly SemaphoreSlim _semaphore = new(1, 1);

private async Task DoAsync()
{
    await Task.Run(async () =>
    {
        bool acquired = _semaphore.Wait(0);
        if (acquired)
        {
            var delayTask = Task.Delay(5000);
            try
            {
                DoStuff();
            }
            finally
            {
                await delayTask;
                _semaphore.Release();
            }
        }
    });
}

This way the ThreadPool thread will be utilized only during the DoStuff execution, and then it will be freed, and it will be available for other work.

Creating the Task.Delay(5000) task before starting the DoStuff and awaiting it afterwards, has the generally desirable effect of including the duration of the DoStuff into the 5 seconds delay. If you don't want this behavior, you can just create and await the task in the same line: await Task.Delay(5000);

Nice. I would put the `Wait` and the `Delay` inside the `try` also, that way you could catch a rude thread abort, or the very slim chance of `Delay` throwing, it also deals with the addition of a `CancellationToken` — Charlieface, Aug 10 '21 at 19:03
@Charlieface AFAIK the `SemaphoreSlim` is not designed to be durable in the face of an aborted thread, like the `Monitor`. In all examples I've seen, the `Wait`/`WaitAsync` happens before entering the `try` block. If you move the `Wait` inside the `try` block, the semaphore could be `Release`d without being acquired, resulting to a [`SemaphoreFullException`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.semaphorefullexception) or to buggy behavior. — Theodor Zoulias, Aug 10 '21 at 19:10
Thank you very much @TheodorZoulias for your time and help. This is what I was looking for. — JP Garza, Aug 10 '21 at 19:42

Is this good practice use of an async task with monitor and sleep?

1 Answers1