10

I am working on a web application, where several users can update the same record. So to avoid a problem if users are updating the same record at the same time, I am saving their changes in a queue. When each save occurs, I want to call a method that processes the queue on another thread, but I need to make sure that the method cannot run in another thread if it is called again. I’ve read several posts on the subject, but not sure what is best for my situation. Below is the code I have now. Is this the correct way to handle it?

public static class Queue {
    static volatile bool isProcessing;
    static volatile object locker = new Object();

    public static void Process() {
        lock (locker) {
            if (!isProcessing) {
                isProcessing = true;

                //Process Queue...

                isProcessing = false;
            }
        }
    }
}
horgh
  • 17,918
  • 22
  • 68
  • 123
user2628438
  • 169
  • 1
  • 2
  • 8
  • 2
    Databases know how to avoid simultaneous update problems. Why reinvent the wheel? – John Saunders Dec 28 '13 at 01:49
  • The data is being stored in Azure tables. The processing can take some time between the request for the original record, and the update. The storage client would just throw an error if another user has updated since. – user2628438 Dec 28 '13 at 04:38
  • 1
    @user2628438 I have updated my answer for abort-safety. That means that if the thread that has the lock is aborted (something you shouldn't do anyway) another thread can enter. I did also fix a bug in the second example (if `TryEnter` succeeds I have to call `Exit` to finish the critical section). Note: `lock` is abort-safe since C# 4.0. – Theraot Dec 28 '13 at 07:24
  • @Theraot, what is the `_syncroot` object in the `Monitor.TryEnter` example? Or should I use the `locker` object there? – user2628438 Dec 28 '13 at 14:20
  • @user2628438 yes that would be `locker`. I've updated that. `syncroot` is a common name for an object used in `Monitor`, because that's how Microsoft called them when they thought it was good idea to expose them. You can still find classes that expose `SyncRoot` those are from the times of .NET 2.0 or prior (it is part of the `ICollection` interface, the not generic one). Should you use the `SyncRoot` exposed by those classes? No. – Theraot Dec 28 '13 at 19:40

2 Answers2

17

New answer

If you are persisting these records to a database (or data files, or similar persistence system) you should let that underlying system handle the synchronization. As JohnSaunders pointed out Databases already handle simultaneous updates.


Given you want to persist the records… the problem presented by John is that you are only synchronizing the access to the data in a single instance of the web application. Still, there could be multiple instances running at the same time (for example in a server farm, which may be a good idea if you have high traffic). In this scenario using a queue to prevent simultaneous writes is not good enough because there is still a race condition among the multiple instances of the web page.

In that case, when you get updates for the same record from different instances, then the underlying system will have to handle the collision anyway, yet it will not be able to do it reliably because the order of the updates has been lost.

In addition to that problem, if you are using this data structure as a cache, then it will provide incorrect data because it is not aware of the updates that happen in another instance.


With that said, for the scenarios where it may be worth to use a Thread-Safe Queue. For those cases you could use ConcurrentQueue (as I mention at the end of my original answer).

I'll keep my original answer, because I see value in helping understand the threading synchronization mechanism available in .NET (of which I present a few).


Original answer

Using lock is enough to prevent the access of multiple threads to a code segment at the same time (this is mutual exclusion).

Here I have commented out what you don't need:

public static class Queue {
    // static volatile bool isProcessing;
    static /*volatile*/ object locker = new Object();

    public static void Process() {
        lock (locker) {
            // if (!isProcessing) {
            //  isProcessing = true;

                //Process Queue...

            //  isProcessing = false;
            // }
        }
    }
}

The lock does NOT need volatile to work. However you might still need the variable to be volatile due to other code not included here.


With that said, all the threads that try to enter in the lock will be waiting in a queue. Which as I understand is not what you want. Instead you want all the other threads to skip the block and leave only one do the work. This can be done with Monitor.TryEnter:

public static class Queue
{
    static object locker = new Object();

    public static void Process()
    {
        bool lockWasTaken = false;
        try
        {
            if (Monitor.TryEnter(locker))
            {
                lockWasTaken = true;
                //Process Queue…
            }
        }
        finally
        {
            if (lockWasTaken)
            {
                Monitor.Exit(locker);
            }
        }
    }
}

Another good alternative is to use Interlocked:

public static class Queue
{
    static int status = 0;

    public static void Process()
    {
        bool lockWasTaken = false;
        try
        {
            lockWasTaken = Interlocked.CompareExchange(ref status, 1, 0) == 0;
            if (lockWasTaken)
            {
                //Process Queue…
            }
        }
        finally
        {
            if (lockWasTaken)
            {
                Volatile.Write(ref status, 0);
                // For .NET Framework under .NET 4.5 use Thread.VolatileWrite instead.
            }
        }
    }
}

Anyway, you don't have the need to implement your own thread-safe queue. You could use ConcurrentQueue.

Theraot
  • 31,890
  • 5
  • 57
  • 86
  • -1: not good enough in a web application. There could be multiple copies of the AppDomain running in the case of a recycle, or in the case of a web farm or web garden. – John Saunders Dec 28 '13 at 01:49
  • @JohnSaunders are you telling me that multiple AppDomains are sharing static fields? – Theraot Dec 28 '13 at 02:01
  • No, I'm telling you that there could be multiple AppDomains running the same code at the same time, so multiple copies of the static, meaning no synchronization among the multiple AppDomains. – John Saunders Dec 28 '13 at 02:02
  • @JohnSaunders yes, still as it is not the same data. It makes no sense to try to sync them at this level. If we are working on data that needs to be kept across AppDomains then - as you have suggested - using a database is a better idea. I'm not sure if that's what user2628438 wants. – Theraot Dec 28 '13 at 02:04
  • 1
    Why would it not be the same data? It's the same web application, and presumably the same set of users. – John Saunders Dec 28 '13 at 02:07
  • Allow me to express it another way: We agree that the data on this class is not shared across AppDomains, meaning that each AppDomain has a copy. That's what I mean by "not the same data". What I understand you are saying is that keeping this data in memory is not good enough because it will have an instance per AppDomain. And I say, maybe that's user2628438 wants. Ok, an example where you have multiple instances and you don't want to share this data: maybe this are game servers, and each user connects to a server to play. By nature of the game it may be appropiate to kept them appart. – Theraot Dec 28 '13 at 02:18
  • Actually, I'm thinking more broadly. During these situations of multiple AppDomains, there would be two copies of the data, as you have stated, but the original reason for using the queue was to prevent simultaneous update. In this case, simultaneous update is not prevented because there will be two sets of data. User A could update record #100 in one AppDomain, and User B could update the same record in another AppDomain. The locks would prevent simultaneous access to each of the two copies, but the two copies would not be synchronized. This would be a problem - they represent the same record – John Saunders Dec 28 '13 at 02:31
  • @JohnSaunders, couldn't you get around that with a named `Mutex` or `Semaphore` provided that your applications are running on a single system as opposed to a web farm/garden? – Kirill Shlenskiy Dec 28 '13 at 02:41
  • @KirillShlenskiy: yes, but you might still need to synchronize two copies of the data. Really. Better to let a professional deal with this. Databases are the professionals in this area. – John Saunders Dec 28 '13 at 03:00
  • @Theraot, thanks for this. I think your original answer will work for me. Data will be stored in Azure tables. I could be wrong, but I don’t think Azure tables would be able to handle this situation for me. – user2628438 Dec 28 '13 at 04:45
  • @JohnSaunders, Thanks for bringing up the multiple instance issue, I didn’t even think of that. Right now it will only be a single instance so Theraot’s answer will work, but if we need to scale later on, this wouldn’t be the right approach. – user2628438 Dec 28 '13 at 04:48
  • Is the `volatile` keyword necessary for the `object` example? – trinalbadger587 Jan 05 '22 at 00:22
  • @trinalbadger587 Here `volatile` is not necessary. The `lock` per-se does not need `volatile`, what happens inside might. Here `volatile` is only disabling some reordering optimizations. For example, either the compiler or branch prediction may go ahead an set a variable before the `lock`, because they determine it will be set anyway. Which could have been the case of `isProcessing`. – Theraot Jan 05 '22 at 03:33
  • @trinalbadger587 By the way, I really don't like relying on `volatile`. The fact that it can be hard to tell if we need it is an issue, worsen by it sitting away from the threading code. That's bad separation of concerns. Plus, I've been conditioned to not trust it due to some bugs on early versions of .NET. I imagine a code review where we go "hmm… I'm not sure if we need `volatile`, let us test", it does not fail on tests, and we remove it introducing a hard to reproduce and to debug regression bug. I rather build threading code on `Interlocked` and similar (e.g. `VolatileWrite`) every time. – Theraot Jan 05 '22 at 03:42
  • @trinalbadger587 Ok, "every time" may come across wrong. Use the threading solutions available on the standard library (i.e. `Monitor`, `Semaphore`, `ReaderWriterLock` and so on), I have nothing against those. It just happens that I have re-implemented plenty of those as backports for early .NET versions. And `Interlocked` is both safe and versatile approach to do so, and it works on every version of .NET. So if there is nothing in the library for my use case, that is my go-to option. – Theraot Jan 05 '22 at 03:52
  • Also since the `locker` is protecting `static` methods, it should probably be `static` as well. – Theodor Zoulias Jan 05 '22 at 04:31
1

A lock is good but it won't work for async await. You will get the following error if you try to await a method call in a lock:

CS1996 Cannot await in the body of a lock statement

In this case you should use a SemaphoreSlim

Example:

public class TestModel : PageModel
{
    private readonly ILogger<TestModel> _logger;
    private static readonly SemaphoreSlim _semaphoreSlim = new SemaphoreSlim(1, 1);

    public TestModel(ILogger<TestModel> logger)
    {
        _logger = logger;
    }

    public async Task OnGet()
    {
        await _semaphoreSlim.WaitAsync();
        try
        {
            await Stuff();
        }
        finally
        {
            _semaphoreSlim.Release();
        }
    }
}

It is important to not new SemaphoreSlim in the constructor or anywhere else because then it won't work.

https://stackoverflow.com/a/18257065/3850405

https://learn.microsoft.com/en-us/dotnet/api/system.threading.semaphoreslim?view=net-5.0

Ogglas
  • 62,132
  • 37
  • 328
  • 418
  • But other thread will waiting Release() and still executing? Can It make other threads ignore if SemaphoreSlim in taken? – dellos Nov 19 '22 at 10:05